Clean up stale JDK 21 references left over from the JDK 25 bump - #19103
Closed
yashmayya wants to merge 1 commit into
Closed
Clean up stale JDK 21 references left over from the JDK 25 bump#19103yashmayya wants to merge 1 commit into
yashmayya wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19103 +/- ##
============================================
+ Coverage 65.51% 65.52% +0.01%
Complexity 1423 1423
============================================
Files 3430 3430
Lines 218058 218058
Branches 34651 34651
============================================
+ Hits 142856 142891 +35
+ Misses 63640 63608 -32
+ Partials 11562 11559 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The JDK 25 bump (#19014) left a number of JDK 21 references behind. Most are cosmetic, but two are functional breakages. Follow-up to #19102.
Functional fixes
docker/images/pinot/docker-build.shcould not build Pinot at all. With no 4th argument it defaulted toJAVA_VERSION=21, which setsPINOT_BASE_IMAGE_TAG=21-amazoncorretto. That image ships Corretto 21.0.11, and the root pom's enforcer requires[25,), so the build failed invalidate. Verified locally by running Pinot's exactrequireJavaVersionrule inside both base images:.pinot_base_docker_image_build_and_push.shcould publish a mislabeled base image. It passed--build-arg JAVA_VERSION=${JDK_VERSION:-21}, and the image contents come fromJDK_VERSIONwhile the published tag comes from the independentTAGvariable. An invocation withTAG=25-amazoncorrettoandJDK_VERSIONunset would have built a JDK 21 image, succeeded, and pushed it aspinot-base-runtime:25-amazoncorretto— a base image whose JRE cannot load Pinot'srelease=25bytecode. This isworkflow_dispatch-only so CI always set the variable, but the fallback made the failure silent and unrecoverable once pushed. Now hard-fails when unset, matching.pinot_compile_and_push_build_image.shand.pinot_package_single_platform.sh. Note this--build-argalso overrode the dockerfileARGdefaults, so fixing those without this would have had no effect through the only script that builds them.Stale configuration
build-pinot-base-docker-image.yml: dropped21from both matrices. This stops publishingpinot-base-{build,runtime}:21-*, so those tags freeze and stop receiving OS/JDK CVE patches — worth a line in the release notes. Nothing references them: every consumer derives its tag from a JDK-25-only source.pinot-basedockerfiles:ARG JAVA_VERSIONdefault 21 → 25.AGENTS.md(3 places; its counterpart inCLAUDE.mdwas already updated, so the two disagreed andAGENTS.mdcontradicted itself),.github/copilot-instructions.md,docker/images/pinot/README.md, andMseBlock.java's class Javadoc.pinot-clients/pinot-cli/README.mdsaid "Java 21+ on PATH". That one was wrong rather than conservative:pinot-clicompiles at${jdk.version}with norelease=11override, so the shipped CLI throwsUnsupportedClassVersionErroron a JDK 21 PATH.Dead JDK version guard in the launcher template
appAssemblerScriptTemplategated the--add-opensflags behindif [ "$(jdk_version)" -gt 11 ], which can no longer be false. Removing it also fixes a latent bug:jdk_version()probed barejavaonPATHrather than the$JAVACMDactually exec'd, so a stale or missing PATHjavamade the guard false (or printedinteger expression expected) and silently dropped the module-access flags while running on 25 — producing exactly theInaccessibleObjectExceptionthose flags exist to prevent. It also forked a JVM on every script invocation.Verified behavior-identical: compared the old and new logic across
{JAVA_OPTS unset, set, set + PLUGINS_INCLUDE}(byte-identicalALL_JAVA_OPTSin all three), then built-Pbin-distand traced the real generatedpinot-admin.shto confirm the exec line still carries all six--add-opens, both Netty properties and-Dplugins.dir.bin/pinot-admin.sh -helpruns clean.Deliberately not fixed here
Removing the guard exposes that this assignment overwrites
@EXTRA_JVM_ARGUMENTS@instead of appending, so the per-programjvmSettingsinpinot-tools/pom.xml— heap sizes,-Dlog4j2.configurationFile,-Dpinot.admin.system.exit— are discarded wheneverJAVA_OPTSis unset. That is long-standing behavior (the guard was always true, so they were already being dropped), and appending them would change the effective heap, logging config and admin exit behavior of every generated script. It needs to be a deliberate change, not a side effect of a cleanup, so it is left as-is with a comment. Happy to file a separate issue if that's preferred.I also kept the now-provably-dead
if [ -z "$JAVA_OPTS" ]block rather than deleting it: removing it would drop the@EXTRA_JVM_ARGUMENTS@token from the template entirely and leave the pom'sjvmSettingswith no representation at all, making that config silently inert and foreclosing the fix above.Stale JDK 21 mentions in
kb/andpinot-perf/README.mdare left for a separate pass — they describe agent/benchmark guidance rather than build or runtime configuration.Testing
Beyond the enforcer probe and script verification above, built both runtime base images with the new default ARG (no
--build-arg) and confirmed the JDK inside:The
ms-openjdkfiles re-declareARG JAVA_VERSIONwithout a value in the second stage, which correctly inherits the pre-FROMdefault — hence only one default needed changing there versus two in the corretto files. Confirmed the newJDK_VERSIONguard exits 1 before invokingdocker build.spotless,checkstyleandlicense:checkpass onpinot-tools,pinot-query-runtimeandpinot-cli;pinot-query-runtimetest-compiles clean.I did not rebuild the
pinot-base-buildimages locally (the corretto one compiles Thrift from source); their JDK 25 variants are already published by CI, and the JDK acquisition steps are identical to the runtime images verified above.