Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23259 )
Change subject: [build][java] KUDU-3681: Move to JDK17 ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/23259/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23259/1//COMMIT_MSG@7 PS1, Line 7: Move to JDK17 > If we upgrade test environment without this change, would builds pass? In o +1 If this patch isn't compatible with our current pre-commit CI pipeline, what's the plan of updating it (maybe, there is doc with at notes and some random thoughts about that)? In this context what's your opinion on the following: * Does it make sense to do such transition in a few steps/phases, so nothing is broken in the middle: e.g., add JDK11 to the pre-commit CI infra, make it configurable which JDK to use for compilation and test runs, and have Kudu Java build working for both JDK11 and JDK17 in the main branch for some time? AFAIK, current Java code in the main branch of the Kudu project can be built and run against JDK11. * Is it possible to somehow parameterise the minimum required/target JDK version? It might be useful for transitioning our pre-commit CI pipeline as outlined above. As another consideration of lesser importance, it might be useful to maintain the compatibility of our pre-commit CI pipeline with the code in the 1.18.x line since we might need to do another maintenance release in the 1.18.x branch. Of course, we might say that we don't care about pre-commit builds in branch-1.18.x anymore, but then it will be harder to spot problems if/when trying to release 1.18.2: the release manager will need to run all the tests manually, and if we also make the dist-test machines providing only JDK17 environment, then using dist-test for that isn't an option. http://gerrit.cloudera.org:8080/#/c/23259/1//COMMIT_MSG@11 PS1, Line 11: The changes: It's nice to have this summary. However, I think several pieces of important information are not mentioned here. At least, consider shedding some light in the description of this changelist on the following: * Is it still possible to compile Kudu with JDK8 with this patch if not those Hive and jacoco dependency upgrades? * Is it still possible to compile Kudu with JDK11 with this patch? * Beyond necessary updates of release notes, docs, etc., are any other updates needed/planned just in the Gradle-related department and Java code itself to support JDK17, or that's the only change you are planning to have? http://gerrit.cloudera.org:8080/#/c/23259/1//COMMIT_MSG@18 PS1, Line 18: upgrade the Hive dependency to 4.1.0 > no, without upgrading the Hive dependency Kudu won't compile with JDK17 Is it possible to do it in the reverse order: first upgrade the hive dependency to 4.1.0 (which compiles with JDK11 AFAIK), and then add this changelist on top of that? The same if for jacoco. As of now, Kudu compiles with JDK11, so I'd think it's possible to use that as a stop-gap for all the necessary preliminary updates/upgrades before adding this patch on top. http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle.properties File java/gradle.properties: http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle.properties@37 PS1, Line 37: javaCompatibility I'm confused: why to keep this if its former primary usage in compile.gradle is now removed as of PS1? I see there are still references to javaCompatibility property in gradle/quality.gradle and gradle/docs.gradle, but I'm not quite sure whether it's just something that's been missed updating in this patch or that's how it's supposed to be? http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/compile.gradle File java/gradle/compile.gradle: http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/compile.gradle@22 PS1, Line 22: // --release is the recommended way to select the target release, but it's only supported in : // Java 9+ so we also set -source and -target via `sourceCompatibility` and `targetCompatibility`. If this statement is true, and the 'release' option "... takes precedence over source/targetCompatibility ..." as you added below, why keeping the sourceCompatibility and targetCompatibility if we are switching to Java 17 as of this changelist? Or we are still leaving the option to compile with JDK8? Otherwise, why to still have those 'if (JavaVersion.current().isJava9Compatible()) {}' clauses? http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/compile.gradle@26 PS1, Line 26: sourceCompatibility = JavaVersion.current().toString() : targetCompatibility = JavaVersion.current().toString() I'm confused: what if running the build now with this on JDK8? Wouldn't it set this to something JDK8-ish? Then why all the talk about moving to JDK17? http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/dependencies.gradle File java/gradle/dependencies.gradle: http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/dependencies.gradle@44 PS1, Line 44: 4.1.0 Why do we need to upgrade this in the same changelist when switching to JDK17? Is it possible to do this in a separate follow-up changelist? http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/tests.gradle File java/gradle/tests.gradle: http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/tests.gradle@49 PS1, Line 49: JavaVersion.current() <=> JavaVersion.VERSION_17 Please add a comment on why using this JVM option in JDK17 and newer. Per information published at https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html, I guess all the related warnings are now covered by corresponding '--add-opens' flags below, right? Or currenly some of Kudu tests fail because of InaccessibleObjectException thrown even with all the '--add-opens' below? http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/tests.gradle@74 PS1, Line 74: JavaVersion.current().isJava9Compatible() If this is JDK17-specific stuff, why not to add it for JDK17 only, or for JDK17 and newer? Also, there are some shenanigans with 'reflectionModules' above -- why not to use similar approach for these instead of all this 'jvmArgs +=' boilerplate? BTW, many of these are already added, and your newly added lines are just duplicates. E.g., take a look at "java.base/java.io", "java.base/java.lang", etc. in 'reflectionModules' above. http://gerrit.cloudera.org:8080/#/c/23259/1/java/kudu-spark/build.gradle File java/kudu-spark/build.gradle: http://gerrit.cloudera.org:8080/#/c/23259/1/java/kudu-spark/build.gradle@35 PS1, Line 35: testImplementation project(path: ":kudu-client") I'm surprised seeing this extra dependency popping up here. Is this just because of JDK17 upgrade? Why it wasn't necessary with JDK8 and JDJ11? What's going on here? -- To view, visit http://gerrit.cloudera.org:8080/23259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f73c0d3a79e1c37b12a173881273b4b68d718b3 Gerrit-Change-Number: 23259 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Fri, 05 Sep 2025 23:35:36 +0000 Gerrit-HasComments: Yes
