Raise Pinot build baseline to JDK 17#18042
Closed
xiangfu0 wants to merge 1 commit intoapache:masterfrom
Closed
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18042 +/- ##
============================================
+ Coverage 63.32% 63.34% +0.02%
Complexity 1543 1543
============================================
Files 3200 3200
Lines 194209 194207 -2
Branches 29932 29932
============================================
+ Hits 122986 123029 +43
+ Misses 61557 61521 -36
+ Partials 9666 9657 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
added
java
xiangfu0
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Raises Apache Pinot’s default build baseline from Java 11 to Java 17 and aligns supporting tooling/docs/CI to stop hardcoding JDK 11.
Changes:
- Updates the root Maven build baseline to JDK 17 and removes remaining module-level Java 11 compiler overrides.
- Updates Docker defaults and GitHub Actions workflows to use JDK 17 (and tags/images aligned to 17).
- Removes
-Djdk.version=11forcing from helper scripts and updates various docs to reflect the new baseline.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Changes the repo-wide jdk.version default to 17, impacting compiler/javadoc/plugin defaults. |
pinot-timeseries/pinot-timeseries-planner/pom.xml |
Removes Java 11 compiler overrides so the module inherits the new baseline. |
pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql/pom.xml |
Removes Java 11 compiler overrides so the module inherits the new baseline. |
pinot-clients/pinot-cli/README.md |
Updates CLI build requirements from Java 11+ to Java 17+. |
helm/pinot/values.yaml |
Updates Helm chart documentation comments to reflect Corretto 17 as default. |
docker/images/pinot/README.md |
Updates Docker build script usage/docs to JDK 17 baseline (but still mentions Kafka version). |
docker/images/pinot/Dockerfile.package |
Updates default build/runtime image tag from 11-* to 17-*. |
docker/images/pinot/Dockerfile.build |
Updates default base image tag and default JDK_VERSION to 17 for build image. |
docker/images/pinot/Dockerfile |
Updates default base image tag and default JDK_VERSION to 17 for full image build. |
docker/images/pinot/docker-build.sh |
Renames/pivots the 5th arg to JDK_VERSION and defaults to 17. |
docker/images/pinot-base/README.md |
Updates documented base-image build commands/tags to 17. |
compatibility-verifier/checkoutAndBuild.sh |
Stops forcing -Djdk.version=11 so each checked-out revision uses its own configured baseline. |
CLAUDE.md |
Updates build guidance to JDK 17+ and “code targets Java 17”. |
AGENTS.md |
Updates build/runtime guidance to Java 17+ and “code targets Java 17”. |
.github/workflows/scripts/docker/.pinot_single_platform_docker_image_build.sh |
Updates default JDK to 17 and switches “latest” tagging condition to 17-amazoncorretto. |
.github/workflows/scripts/docker/.pinot_multi_arch_docker_image_manifest_package.sh |
Switches “latest” manifest condition to 17-amazoncorretto. |
.github/workflows/scripts/docker/.pinot_base_docker_image_build_and_push.sh |
Updates default JAVA_VERSION build-arg to 17. |
.github/workflows/scripts/.pinot_quickstart.sh |
Removes -Djdk.version=11 forcing and updates build log messaging. |
.github/workflows/pinot_tests.yml |
Updates CI setup steps/matrices from Java 11/21 to 17/21. |
.github/workflows/pinot_multi_stage_query_engine_compatibility_tests.yml |
Updates CI setup from JDK 11 to JDK 17. |
.github/workflows/pinot_compatibility_tests.yml |
Updates CI setup from JDK 11 to JDK 17. |
.github/workflows/build-pinot-base-docker-image.yml |
Updates Docker base-image workflow tagging/versioning to 17. |
.github/workflows/build-multi-arch-pinot-docker-image.yml |
Updates base-image tags to 17 and passes JDK_VERSION=17 to build script. |
.github/copilot-instructions.md |
Updates stated primary Java version to 17 (but retains a Java 8 claim for pinot-clients). |
Comments suppressed due to low confidence (1)
.github/workflows/build-multi-arch-pinot-docker-image.yml:89
.github/workflows/scripts/docker/.pinot_single_platform_docker_image_build.shrequiresRUNTIME_IMAGE_TAGSto decide which runtime images to build/push, but this workflow step doesn’t set it. As a result, the script will only build the intermediatepinot-build:*image and skip building/pushing any finalapachepinot/pinot:*runtime images. SetRUNTIME_IMAGE_TAGShere (e.g., to match the supported runtime tags) or adjust the script to default to${BASE_IMAGE_TAG}when unset.
- name: Build and push the Docker image
env:
DOCKER_FILE_BASE_DIR: "docker/images/pinot"
DOCKER_IMAGE_NAME: ${{ github.event.inputs.dockerImageName }}
BUILD_PLATFORM: "linux/${{ matrix.arch }}"
BASE_IMAGE_TAG: ${{ matrix.base-image-tag }}
JDK_VERSION: "17"
PINOT_GIT_URL: ${{ github.event.inputs.gitUrl }}
PINOT_BRANCH: "${{needs.generate-build-info.outputs.commit-id}}"
TAGS: "${{needs.generate-build-info.outputs.tags}}"
run: .github/workflows/scripts/docker/.pinot_single_platform_docker_image_build.sh
Comment on lines
+59
to
+61
| echo "Trying to build Pinot docker image from Git URL: [ ${PINOT_GIT_URL} ] on branch: [ ${PINOT_BRANCH} ] and tag it as: [ ${DOCKER_TAG} ]. Kafka Dependencies: [ ${KAFKA_VERSION} ]. JDK Version: [ ${JDK_VERSION} ]." | ||
|
|
||
| docker build --no-cache -t ${DOCKER_TAG} --build-arg PINOT_BRANCH=${PINOT_BRANCH} --build-arg PINOT_GIT_URL=${PINOT_GIT_URL} --build-arg KAFKA_VERSION=${KAFKA_VERSION} --build-arg JAVA_VERSION=${JAVA_VERSION} -f Dockerfile . | ||
| docker build --no-cache -t ${DOCKER_TAG} --build-arg PINOT_BRANCH=${PINOT_BRANCH} --build-arg PINOT_GIT_URL=${PINOT_GIT_URL} --build-arg KAFKA_VERSION=${KAFKA_VERSION} --build-arg JDK_VERSION=${JDK_VERSION} -f Dockerfile . |
There was a problem hiding this comment.
KAFKA_VERSION is accepted and passed as a --build-arg, but the referenced docker/images/pinot/Dockerfile doesn’t define or use ARG KAFKA_VERSION (only JDK_VERSION). This makes the Kafka Dependencies argument misleading and effectively dead code; either remove the KAFKA_VERSION arg from this script/README or wire it into the Docker build if it’s still required.
Comment on lines
31
to
+47
| @@ -44,11 +44,7 @@ The docker image is tagged as `[Docker Tag]`. | |||
|
|
|||
| `Kafka Version`: The Kafka Version to build pinot with. Default is `2.0` | |||
|
|
|||
| `Java Version`: The Java Build and Runtime image version. Default is `11` | |||
|
|
|||
| `JDK Version`: The JDK parameter to build pinot, set as part of maven build option: `-Djdk.version=${JDK_VERSION}`. Default is `11` | |||
|
|
|||
| `OpenJDK Image`: Base image to use for Pinot build and runtime. Default is `openjdk`. | |||
| `JDK Version`: The JDK parameter to build pinot, set as part of maven build option: `-Djdk.version=${JDK_VERSION}`. Default is `17` | |||
There was a problem hiding this comment.
The usage/docs still present Kafka Version as a required/meaningful parameter, but docker/images/pinot/Dockerfile doesn’t consume KAFKA_VERSION at all. Consider either removing Kafka Version from the documented interface (and examples) or updating the Docker build to actually use it, so the README matches the implementation.
|
|
||
| - Most source code is written in Java 11. | ||
| - Most source code is written in Java 17. | ||
| - Code in `pinot-clients` is written in Java 8. |
There was a problem hiding this comment.
After raising the repo-wide baseline to Java 17, this doc still states that pinot-clients is written in Java 8. Since maven.compiler.release/source/target now default to 17 repo-wide, this is ambiguous and may be inaccurate for build/runtime expectations. Please clarify whether pinot-clients is still intended to be Java 8 bytecode-compatible (e.g., built with --release 8) or update the statement to match the new baseline.
Suggested change
| - Code in `pinot-clients` is written in Java 8. | |
| - Code in `pinot-clients` is also built for Java 17. |
Contributor
|
Why don't we directly upgrade to 21? |
Contributor
Author
|
yes, give another try on jdk 21 :) |
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
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.
Summary
-Djdk.version=11across checked-out revisionsRoot cause
Pinot already compiled successfully on JDK 17 when the build was explicitly forced to
-Djdk.version=17, but the repository default still targeted Java 11 and several supporting workflows/scripts still hard-pinned JDK 11. That left the project in a state where JDK 17 worked only as an override instead of as the default baseline.Developer impact
After this change, building Pinot on JDK 17 works without passing a JDK override flag, and the surrounding CI / Docker / quickstart configuration is aligned with that new baseline.
How to reproduce the issue
JAVA_HOME.masterbefore this patch, run./mvnw -T1C -Ppinot-fastdev -DskipTests -Dskip.integration.tests=true -Dcheckstyle.skip=true -Dspotless.check.skip=true -Dlicense.skip=true compile.-Djdk.version=17to exercise a JDK 17 build baseline.Validation
./mvnw -T1C -Ppinot-fastdev -DskipTests -Dskip.integration.tests=true -Dcheckstyle.skip=true -Dspotless.check.skip=true -Dlicense.skip=true compile./mvnw -pl "pinot-timeseries/pinot-timeseries-planner,pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql,pinot-clients/pinot-cli" -am spotless:apply./mvnw -pl "pinot-timeseries/pinot-timeseries-planner,pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql,pinot-clients/pinot-cli" -am checkstyle:check./mvnw -pl "pinot-timeseries/pinot-timeseries-planner,pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql,pinot-clients/pinot-cli" -am license:format./mvnw -pl "pinot-timeseries/pinot-timeseries-planner,pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql,pinot-clients/pinot-cli" -am license:check