Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24040 )
Change subject: [infra] Update build-and-test.sh for Java 17 ...................................................................... Patch Set 9: (4 comments) http://gerrit.cloudera.org:8080/#/c/24040/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24040/9//COMMIT_MSG@12 PS9, Line 12: "JAVA_8 _OVERRIDE" (written without the space) https://gerrit.cloudera.org/#/c/24040/9/build-support/jenkins/build-and-test.sh@355 LOL http://gerrit.cloudera.org:8080/#/c/24040/9/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/24040/9/build-support/jenkins/build-and-test.sh@349 PS9, Line 349: JAVA_8_OVERRIDE nit: why not JAVA8_OVERRIDE? There are JAVA8_HOME and JAVA17_HOME variables, but for some reason it's JAVA_8_OVERRIDE -- is it intentional using different convention for underscores in the variable name? http://gerrit.cloudera.org:8080/#/c/24040/9/build-support/jenkins/build-and-test.sh@355 PS9, Line 355: JAVA_8_OVERRIDE As for special label/tags in a text, usually it makes sense to require a particular placing of those, otherwise there isn't a way to mention JAVA_8_OVERRIDE in the message to be interpreted any other way (e.g., imagine the description of a follow-up changelist to update this script w.r.t. handling the JAVA_8_OVERRIDE tag). For example, maybe require that JAVA_8_OVERRIDE is present in the very beginning of a string in the commit message? http://gerrit.cloudera.org:8080/#/c/24040/9/java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java File java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java: http://gerrit.cloudera.org:8080/#/c/24040/9/java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java@150 PS9, Line 150: javaVersion.equals("17") readability nit: make this a top-level condition and move everything related under the corresponding 'if ()' scope Also, maybe it makes sense to report on an empty string (e.g., log a warning) for easier troubleshooting? -- To view, visit http://gerrit.cloudera.org:8080/24040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id1d6d65fbd58a8c96d1dca958fc66207893bb99e Gerrit-Change-Number: 24040 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Mon, 02 Mar 2026 21:49:48 +0000 Gerrit-HasComments: Yes
