Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/23259 )
Change subject: [build][java] KUDU-3681: Move to JDK17 ...................................................................... Patch Set 1: (14 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 > +1 Regarding the pre-commit CI infra, it does need changes. There is a tool called 'jenv'[0] which I've used locally, it's very similar to 'venv' for Python, it basically can set the Java version to be used locally (e.g. current directory). We could add it's usage to the Java testing scripts, and it would basically enable us to do testing on multiple Java versions, so older branches could still run JDK8 for maintenance releases. I don't have access to the Jenkins job configurations and to the Gerrit configs, but I'm happy to make these changes. Regarding the interim JDK11 builds, unfortunately Hive made sort of a breaking change by moving to JDK17, and it is not backwards compatible, not even with JDK11, I haven't tested it, but the Hive devs I've talked with said that it is unlikely to be working and can likely break, so it probably wouldn't be stable. Hive doesn't even run their github actions with Java11 [1] anymore. [0] https://github.com/jenv/jenv [1] https://github.com/apache/hive/commit/413069ea0faeb893fe78faa215b08a70ece80595 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 importa * No, I attempted to make the change so that we are JDK8 backwards compatible. Unfortunately due to Hive's backwards incompatibility compilation with JDK8 is no longer possible (if the Hive and Jacoco dependencies are updated as mentioned) * No, it isn't * No Gradle upgrade is needed for JDK17, for JDK21, we would need to upgrade Gradle to 8.x, but I think we should not do a Gradle upgrade at this point, it is unnecessary and as seen before, Gradle upgrade can cause all sorts of build/publish/etc issues. http://gerrit.cloudera.org:8080/#/c/23259/1//COMMIT_MSG@12 PS1, Line 12: javaCompatibility to 17 in gradle.properties > Do you think System requirements also need to change in java/README.adoc? Yes definitely, I was considering updating the docs in a separate changeset, do you think I should include those changes here? Also I didn't wan to start the documentation before the actual change/switchover was discussed an finalized. http://gerrit.cloudera.org:8080/#/c/23259/1//COMMIT_MSG@18 PS1, Line 18: upgrade the Hive dependency to 4.1.0 > Is it possible to do it in the reverse order: first upgrade the hive depen Hive 4.1.0 doesn't compile with JDK 11, do you have a source that states otherwise? 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.gradl right, this got left in, I'll clean it up 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 o Originally I wanted to keep JDK8 backwards compatibility but the Hive dependency upgrade unfortunately made that impossible. I'll clean these parts up, so that we only set release, it internally sets the target/source compatibility flags: https://www.baeldung.com/java-source-target-options#source-and-target-in-java-9-and-later http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/compile.gradle@24 PS1, Line 24: reelase > nit: fix typo Done 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 removed in PS2 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: > nit: Remove extra whitespace Done 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 JDK It is requied to be able to compile with JDK17, previous versions of Hive do not compile with JDK17. http://gerrit.cloudera.org:8080/#/c/23259/1/java/gradle/dependencies.gradle@111 PS1, Line 111: hadoopHdfs > Is this picked from shadow jar or native system? I'm not sure I understand the question 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. sorry, this was left here by mistake, removed it and re-tested, it's not necessary 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 J the gradle JavaVersion API doesn't have a isJava17Compatible() method, and these are required from Java9+. I moved these to reflectionModules and removed the duplicates https://docs.gradle.org/current/javadoc/org/gradle/api/JavaVersion.html 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 b sorry, this is a leftover of a fix that didn't work/wasn't needed, removing it -- 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: Thu, 11 Sep 2025 17:01:38 +0000 Gerrit-HasComments: Yes
