KAFKA-17811: Separate modules to use different JDKs #17522

frankvicky · 2024-10-17T07:30:05Z

JIRA: KAFKA-17811

This is sub-task to drop broker and tools support for Java 11.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

chia7712

@frankvicky thanks for this patch

chia7712 · 2024-11-17T18:01:44Z

build.gradle

@@ -128,7 +135,11 @@ ext {
      options.compilerArgs << "-Xlint:-serial"
      options.compilerArgs << "-Xlint:-try"
      options.compilerArgs << "-Werror"
-      options.compilerArgs += ["--release", String.valueOf(minJavaVersion)]


Could you please add minClientJavaVersion and minServerJavaVersion?

chia7712 · 2024-11-17T18:08:23Z

build.gradle

@@ -116,6 +116,13 @@ ext {
  configureJavaCompiler = { name, options ->
    // -parameters generates arguments with parameter names in TestInfo#getDisplayName.
    // ref: https://github.com/junit-team/junit5/blob/4c0dddad1b96d4a20e92a2cd583954643ac56ac0/junit-jupiter-params/src/main/java/org/junit/jupiter/params/ParameterizedTest.java#L161-L164
+
+    def modulesNeedingJava17 = [
+      ":core", ":coordinator-common", ":generator", ":group-coordinator",


connect:runtime, connect:mirror, connect:test-plugins, and ``connect:file` should use JDK 17 as well

chia7712 · 2024-11-17T18:08:39Z

build.gradle

+        options.compilerArgs += ["--release", String.valueOf(17)]
+      } else {
+        options.compilerArgs += ["--release", String.valueOf(minJavaVersion)]
+      }
    }


please update scala module as well

chia7712 · 2024-11-17T18:10:28Z

build.gradle

+      ":metadata", ":raft", ":server", ":server-common", ":share", ":storage",
+      ":share-coordinator", ":test-common", ":tools", ":transaction-coordinator"
+    ]
+
    if (name == "compileTestJava" || name == "compileTestScala") {
      options.compilerArgs << "-parameters"
      options.compilerArgs += ["--release", String.valueOf(minJavaVersion)]


this need to be updated too.

chia7712 · 2024-11-22T16:46:02Z

build.gradle


-    scalaCompileOptions.additionalParameters += ["-release", String.valueOf(minJavaVersion)]


streams-scala and streams:test-utils need java 11

chia7712 · 2024-11-22T17:16:55Z

build.gradle

@@ -47,7 +47,8 @@ plugins {

 ext {
  gradleVersion = versions.gradle
-  minJavaVersion = 11
+  minClientJavaVersion = 11
+  minServerJavaVersion = 17
  buildVersionFileName = "kafka-version.properties"


Could you please move modulesNeedingJava11 up to line#52?

modulesNeedingJava11 = [":clients", ":streams", ":streams:test-utils", ":streams-scala"]

chia7712 · 2024-11-22T17:17:08Z

build.gradle

    // -parameters generates arguments with parameter names in TestInfo#getDisplayName.
    // ref: https://github.com/junit-team/junit5/blob/4c0dddad1b96d4a20e92a2cd583954643ac56ac0/junit-jupiter-params/src/main/java/org/junit/jupiter/params/ParameterizedTest.java#L161-L164
-    if (name == "compileTestJava" || name == "compileTestScala") {
+
+    def releaseVersion = isModuleNeedJava11(projectPath) ? minClientJavaVersion : minServerJavaVersion


def releaseVersion = modulesNeedingJava11.any { projectPath == it } ? minClientJavaVersion : minServerJavaVersion

chia7712 · 2024-11-22T17:18:16Z

build.gradle

@@ -726,7 +734,9 @@ subprojects {
  }

  tasks.withType(ScalaCompile) {
-
+    def modulesNeedingJava11 = [":clients", ":streams", ":streams:test-utils", ":streams-scala"]
+    def releaseVersion = modulesNeedingJava11.any { project.path.equals(it) } ?


def releaseVersion = modulesNeedingJava11.any { project.path == it } ? minClientJavaVersion : minServerJavaVersion

frankvicky · 2024-11-23T04:11:18Z

Hi @chia7712
I have updated the PR, PTAL

ijuma · 2024-11-24T05:05:10Z

build.gradle

@@ -47,7 +47,10 @@ plugins {

 ext {
  gradleVersion = versions.gradle
-  minJavaVersion = 11
+  minClientJavaVersion = 11
+  minServerJavaVersion = 17


I think it would be more accurate to say minNonClientJavaVersion as this includes things like tools and other such modules.

Make sense 😄

minNonClientJavaVersion looks good to me. Additionally, the connect:api module belongs to client APIs, but in KIP-1032 we decided to bump the Connect module to JDK 17. Could you please add a comment for the Connect module?

chia7712

@frankvicky could you please check the version of jars on your local?

frankvicky · 2024-11-24T13:13:16Z

Hi @chia7712
Following is the java version of Consumer.class and BrokerFeatures.class

chia7712 · 2024-11-25T14:15:50Z

@frankvicky Could you please use @SuppressWarnings("removal") to fix the warnings?

> Task :connect:runtime:compileTestJava
/home/chia7712/project/kafka/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/SynchronizationTest.java:464: warning: [removal] SecurityManager in java.lang has been deprecated and marked for removal
            SecurityManager s = System.getSecurityManager();
            ^
/home/chia7712/project/kafka/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/SynchronizationTest.java:464: warning: [removal] getSecurityManager() in System has been deprecated and marked for removal
            SecurityManager s = System.getSecurityManager();
                                      ^
2 warnings

frankvicky · 2024-11-25T15:04:41Z

Hi @chia7712
I have suppressed the removal warning, PTAL 😄

chia7712 · 2024-11-25T16:55:25Z

@frankvicky Could you please update CI file to use JDK 17 and 23

https://github.com/apache/kafka/blob/trunk/.github/workflows/build.yml#L148

chia7712 · 2024-11-26T08:35:32Z

https://github.com/apache/kafka/blob/trunk/build.gradle#L1872

@frankvicky please remove the testRuntimeOnly runtimeTestLibs and add

    libs.slf4jReload4j
    libs.junitPlatformLanucher

chia7712 · 2024-11-26T08:55:34Z

build.gradle

@@ -1869,7 +1874,8 @@ project(':clients') {

    testRuntimeOnly libs.jacksonDatabind
    testRuntimeOnly libs.jacksonJDK8Datatypes
-    testRuntimeOnly runtimeTestLibs
+    testRuntimeOnly libs.slf4jReload4j


Could you please apply this approach on streams module too?

I have applied to 4 modules which need JDK 11

chia7712 · 2024-11-26T11:49:12Z

@frankvicky I think current approach is incorrect. We should make :test-common:test-common-runtime run under JDK 11 to ensure it can offer common helpers to all modules.

Reviewers: Chia-Ping Tsai <[email protected]>

agavra · 2024-12-05T04:26:30Z

Hey @chia7712 @frankvicky it looks like after merging this PR it's no longer possible (or at least straightforward) to run tests in client modules (specifically streams) from within the IDE (IntelliJ specifically). I'm getting:

> Task :test-common:test-common-runtime:compileJava
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':generator:compileJava'.
> error: release version 17 not supported

I've tried manually setting the module in the project settings but that doesn't seem to work. Would be good to document something in the project README.

Note that it still works using ./gradlew (obviously, or CI wouldn't pass) but since it's pretty custom gradle scripting it seems IntelliJ isn't able to properly pick it up. In the meantime I've just reverted this commit locally, but that's obviously not a long-term proper fix for DevX :)

cc @ableegoldman who was running into similar issues

chia7712 · 2024-12-05T05:07:20Z

it's no longer possible (or at least straightforward) to run tests in client modules (specifically streams) from within the IDE (IntelliJ specifically)

Could you please share the details to me? For example, how do you run the tests by IntelliJ?

agavra · 2024-12-05T05:13:29Z

I'm running it like this:

Here's the detailed test configuration:

I don't have any special configurations in IDEA, I just use ./gradlew idea to configure it.

chia7712 · 2024-12-05T05:37:32Z

@agavra Are you using JDK 11 to run the streams tests? If so, that could be an issue since the generator module requires JDK 17. I assume all Kafka developers should use JDK 17, as we typically build the entire project during development.

agavra · 2024-12-05T05:52:05Z

🤦 this is why I shouldn't work at night, I had set the module versions to JDK 17 but not the project version to JDK 17. It now works, thanks @chia7712.

ableegoldman · 2024-12-06T05:40:18Z

I assume all Kafka developers should use JDK 17, as we typically build the entire project during development.

We do indeed need to compile the project with 17, but we should recommend that anyone working on the clients (or any other modules that still support 11) at least set their IDE's language version to JDK11 to avoid accidentally using APIs that don't exist back in 11 (looking at you Optional#isPresent). I also noticed that the PR builds don't show the most clear error when failing due to use of post-JDK-11 APIs :/

chia7712 · 2024-12-08T17:27:19Z

@ableegoldman Thanks for your feedback. I agree that we should prioritize making our developers' experience better. I plan to add sourceCompatibility back to build.gradle since IntelliJ IDEA sets the language level based on sourceCompatibility by default (Gradle Documentation).

The screenshot below demonstrates that IntelliJ IDEA can automatically highlight unsupported usages. With this change, developers won't need to manually configure the language level.

chia7712 · 2024-12-08T17:29:16Z

open KAFKA-18186 to address this.

@ableegoldman please feel free to share your thoughts on this approach.

ijuma · 2024-12-08T18:24:46Z

I see some confusion here and I'll try to clarify it.

Are you using JDK 11 to run the streams tests? If so, that could be an issue since the generator module requires JDK 17. I assume all Kafka developers should use JDK 17, as we typically build the entire project during development.

We should make sure it's possible to run tests with Java 11 for the modules that support it since it's possible to have issues at runtime that only affect a particular version.

We do indeed need to compile the project with 17, but we should recommend that anyone working on the clients (or any other modules that still support 11) at least set their IDE's language version to JDK11 to avoid accidentally using APIs that don't exist back in 11 (looking at you Optional#isPresent)

The expected behavior is that the IDE & gradle plugin support the --release flag which has the correct behavior (it sets the appropriate source and binary versions and ensures the standard library signatures also match it).

I plan to add sourceCompatibility back to build.gradle since IntelliJ IDEA sets the language level based on sourceCompatibility by default (Gradle Documentation).

Are we sure the Gradle plugin doesn't handle this properly? If so, we should add the workaround, but also file a ticket with them. See the following for a similar situation for the Scala (the issue here seems to be for Java instead):

#13205 (comment)

chia7712 · 2024-12-10T00:55:06Z

We should make sure it's possible to run tests with Java 11 for the modules that support it since it's possible to have issues at runtime that only affect a particular version.

that makes sense. open https://issues.apache.org/jira/browse/KAFKA-18192 to handle it.

Are we sure the Gradle plugin doesn't handle this properly? If so, we should add the workaround, but also file a ticket with them. See the following for a similar situation for the Scala (the issue here seems to be for Java instead):

The root cause is that we set the compiler arguments directly instead of using Gradle's recommended options.release. I have left a comment on this pull request.

github-actions bot added build Gradle build or GitHub Actions small Small PRs labels Oct 17, 2024

KAFKA-17811: Separate modules to use different JDKs

1807f63

frankvicky force-pushed the KAFKA-17811 branch from ce2af51 to 1807f63 Compare October 19, 2024 15:13

frankvicky marked this pull request as ready for review October 19, 2024 15:14

frankvicky added 3 commits October 20, 2024 09:15

Merge branch 'trunk' into KAFKA-17811

cd459fd

Merge branch 'trunk' into KAFKA-17811

0b62146

Merge branch 'trunk' into KAFKA-17811

8e739c5

frankvicky added the ci-approved label Oct 26, 2024

frankvicky added 2 commits November 15, 2024 20:17

Merge branch 'trunk' into KAFKA-17811

47631b0

KAFKA-17811: Update build.gradle

de5e1c2

chia7712 reviewed Nov 17, 2024

View reviewed changes

frankvicky added 2 commits November 18, 2024 11:25

KAFKA-17811: refactor build.gradle

694a598

KAFKA-17811: refactor build.gradle

35bc484

frankvicky force-pushed the KAFKA-17811 branch from 3d9b01a to 35bc484 Compare November 22, 2024 16:37

chia7712 reviewed Nov 22, 2024

View reviewed changes

KAFKA-17811: refactor build.gradle

cd4697b

chia7712 reviewed Nov 22, 2024

View reviewed changes

KAFKA-17811: refactor build.gradle

95fa392

ijuma reviewed Nov 24, 2024

View reviewed changes

frankvicky added 3 commits November 24, 2024 13:12

KAFKA-17811: rename variable

1bbf39c

KAFKA-17811: add comments for connect:api

ebeb7f3

Merge branch 'trunk' into KAFKA-17811

6cc7c95

chia7712 reviewed Nov 24, 2024

View reviewed changes

KAFKA-17811: Suppress removal warning

b41f9ab

github-actions bot added the connect label Nov 25, 2024

KAFKA-17811: update build.yml and ci-complete.yml

7b4bac8

frankvicky force-pushed the KAFKA-17811 branch from 82b2549 to 7b4bac8 Compare November 26, 2024 05:49

chia7712 reviewed Nov 26, 2024

View reviewed changes

KAFKA-17811: address comments

2c0c438

frankvicky force-pushed the KAFKA-17811 branch from e1cd08c to 2c0c438 Compare November 26, 2024 09:03

KAFKA-17811: Add :test-common:test-common-runtime to jdk 11 modules

7def29f

chia7712 approved these changes Nov 26, 2024

View reviewed changes

chia7712 merged commit 056a76e into apache:trunk Nov 26, 2024
7 of 8 checks passed

chia7712 mentioned this pull request Nov 26, 2024

KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) #16754

Merged

3 tasks

chiacyu pushed a commit to chiacyu/kafka that referenced this pull request Nov 30, 2024

KAFKA-17811 Separate modules to use different JDKs (apache#17522)

d27c22e

Reviewers: Chia-Ping Tsai <[email protected]>

frankvicky mentioned this pull request Dec 10, 2024

KAFKA-18192: generator module should run under java 11 #18114

Merged

3 tasks

chia7712 mentioned this pull request Dec 10, 2024

KAFKA-18186: set options.release to make Intellij configure suitable language level automatically #18104

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17811: Separate modules to use different JDKs #17522

KAFKA-17811: Separate modules to use different JDKs #17522

frankvicky commented Oct 17, 2024

chia7712 left a comment

chia7712 Nov 17, 2024

chia7712 Nov 17, 2024

chia7712 Nov 17, 2024

chia7712 Nov 17, 2024

chia7712 Nov 22, 2024

chia7712 Nov 22, 2024

chia7712 Nov 22, 2024

chia7712 Nov 22, 2024

frankvicky commented Nov 23, 2024

ijuma Nov 24, 2024

frankvicky Nov 24, 2024

chia7712 Nov 24, 2024

chia7712 left a comment

frankvicky commented Nov 24, 2024

chia7712 commented Nov 25, 2024

frankvicky commented Nov 25, 2024

chia7712 commented Nov 25, 2024

chia7712 commented Nov 26, 2024

chia7712 Nov 26, 2024

frankvicky Nov 26, 2024

chia7712 commented Nov 26, 2024

agavra commented Dec 5, 2024 •

edited

Loading

chia7712 commented Dec 5, 2024

agavra commented Dec 5, 2024

chia7712 commented Dec 5, 2024

agavra commented Dec 5, 2024

ableegoldman commented Dec 6, 2024

chia7712 commented Dec 8, 2024

chia7712 commented Dec 8, 2024

ijuma commented Dec 8, 2024 •

edited

Loading

chia7712 commented Dec 10, 2024


		scalaCompileOptions.additionalParameters += ["-release", String.valueOf(minJavaVersion)]

KAFKA-17811: Separate modules to use different JDKs #17522

KAFKA-17811: Separate modules to use different JDKs #17522

Conversation

frankvicky commented Oct 17, 2024

Committer Checklist (excluded from commit message)

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankvicky commented Nov 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

frankvicky commented Nov 24, 2024

chia7712 commented Nov 25, 2024

frankvicky commented Nov 25, 2024

chia7712 commented Nov 25, 2024

chia7712 commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 commented Nov 26, 2024

agavra commented Dec 5, 2024 • edited Loading

chia7712 commented Dec 5, 2024

agavra commented Dec 5, 2024

chia7712 commented Dec 5, 2024

agavra commented Dec 5, 2024

ableegoldman commented Dec 6, 2024

chia7712 commented Dec 8, 2024

chia7712 commented Dec 8, 2024

ijuma commented Dec 8, 2024 • edited Loading

chia7712 commented Dec 10, 2024

agavra commented Dec 5, 2024 •

edited

Loading

ijuma commented Dec 8, 2024 •

edited

Loading