Conversation
xiangfu0
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18046 +/- ##
============================================
- Coverage 63.53% 63.49% -0.05%
+ Complexity 1658 1657 -1
============================================
Files 3244 3244
Lines 197440 197438 -2
Branches 30557 30557
============================================
- Hits 125437 125355 -82
- Misses 61959 62030 +71
- Partials 10044 10053 +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
force-pushed
the
codex/raise-java-baseline-21
branch
from
4c94562 to
0caf9db
Compare
xiangfu0
force-pushed
the
codex/raise-java-baseline-21
branch
from
0caf9db to
4b2a396
Compare
|
Does this need a review? I'd love to volunteer. Edit: Well, there's not much to review 😄. How can we ship this? |
Just build it internally and see if anything would break. In general the OSS pinot project itself will build if all the CI passed. The main issue will be the company internal tooling and surroundings which requires pinot lib to build |
There was a problem hiding this comment.
Pull request overview
Raises Apache Pinot’s default service build/runtime baseline to Java 21 (with JDK 21+ enforcement), while preserving Java 11 bytecode publication for SPI and Java/JDBC client artifacts, and updates CI/Docker/Helm/docs to reflect the split baseline.
Changes:
- Set root build
jdk.versionto 21 and enforce JDK 21+ at Maven validate time. - Override
jdk.versionto 11 for SPI/common and Java/JDBC client modules to keep Java 11 bytecode output. - Update CI workflows, Docker/Helm defaults, and metrics test layout/dependencies for the new baseline assumptions.
Reviewed changes
Copilot reviewed 42 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Switch default build target to Java 21 and enforce JDK 21+ via enforcer rules. |
pinot-timeseries/pinot-timeseries-spi/pom.xml |
Override module bytecode target to Java 11. |
pinot-timeseries/pinot-timeseries-planner/pom.xml |
Remove explicit Java 11 compiler config so it inherits the Java 21 baseline. |
pinot-spi/pom.xml |
Override module bytecode target to Java 11. |
pinot-segment-spi/pom.xml |
Override module bytecode target to Java 11. |
pinot-common/pom.xml |
Override module bytecode target to Java 11; adjust test deps/excludes related to metrics tests. |
pinot-clients/pinot-java-client/pom.xml |
Override client bytecode target to Java 11. |
pinot-clients/pinot-jdbc-client/pom.xml |
Override client bytecode target to Java 11. |
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/blocks/MseBlock.java |
Update Javadoc to reflect the new Java 21 baseline and potential sealed-type follow-up. |
pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql/pom.xml |
Remove explicit Java 11 compiler config so it inherits the Java 21 baseline. |
pinot-common/src/test/java/org/apache/pinot/common/metrics/MetricValueUtils.java |
Make test metric-value helper implementation-agnostic (no Yammer dependency). |
pinot-plugins/pinot-metrics/pinot-yammer/pom.xml |
Add test dependencies on pinot-common (including test-jar) and other test libs. |
pinot-plugins/pinot-metrics/pinot-dropwizard/pom.xml |
Add test dependencies on pinot-common (including test-jar) and other test libs. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerServerPrometheusMetricsTest.java |
Fix test package path and update config-file relative path. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerMinionPrometheusMetricsTest.java |
Fix test package path and update config-file relative path. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerControllerPrometheusMetricsTest.java |
Fix test package path and update config-file relative path. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerBrokerPrometheusMetricsTest.java |
Fix test package path and update config-file relative path. |
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardServerPrometheusMetricsTest.java |
Fix test package path and factory selection for dropwizard tests. |
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardMinionPrometheusMetricsTest.java |
Fix test package path for dropwizard tests. |
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardControllerPrometheusMetricsTest.java |
Fix test package path for dropwizard tests. |
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardBrokerPrometheusMetricsTest.java |
Fix test package path for dropwizard tests. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/PinotMetricUtilsTest.java |
Add Yammer-focused tests for PinotMetricUtils initialization and cleanup behavior. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/MetricsInspector.java |
Add Yammer test helper for capturing and reading recently-registered metric instances. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/MetricValueUtils.java |
Add Yammer-specific test helper for reading values directly from Yammer registry. |
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/AbstractMetricsTest.java |
Add Yammer-backed tests for AbstractMetrics gauge/timer/meter behaviors and concurrency scenarios. |
pinot-clients/pinot-cli/README.md |
Update CLI requirements to Java 21+. |
helm/pinot/values.yaml |
Update Helm guidance to use Java 21-based images/tags by default. |
docker/images/pinot/docker-build.sh |
Change default Java version to 21 for docker image build script. |
docker/images/pinot/README.md |
Update Docker docs to reflect Java 21 default (but see review comments). |
docker/images/pinot/Dockerfile |
Update Docker build/runtime base tags and default JDK_VERSION to 21. |
docker/images/pinot/Dockerfile.build |
Update Docker build image tag and default JDK_VERSION to 21. |
docker/images/pinot/Dockerfile.package |
Update build/runtime image tags to 21-based tags. |
docker/images/pinot-base/pinot-base-runtime/ms-openjdk.dockerfile |
Update base runtime image default Java version to 21. |
docker/images/pinot-base/pinot-base-runtime/amazoncorretto.dockerfile |
Update base runtime image default Java version to 21. |
docker/images/pinot-base/pinot-base-build/ms-openjdk.dockerfile |
Update base build image default Java version to 21. |
docker/images/pinot-base/pinot-base-build/amazoncorretto.dockerfile |
Update base build image default Java version to 21. |
docker/images/pinot-base/README.md |
Update base-image build instructions/tags to Java 21 variants. |
compatibility-verifier/checkoutAndBuild.sh |
Build compatibility verifier trees with -Djdk.version=21. |
README.md |
Document the new split baseline (services on JDK 21+, SPI/clients target Java 11 bytecode). |
CLAUDE.md |
Update developer guidance to reflect Java 21 service baseline. |
AGENTS.md |
Update developer guidance to reflect Java 21 service baseline. |
.github/workflows/scripts/docker/.pinot_base_docker_image_build_and_push.sh |
Update default base-image Java version to 21. |
.github/workflows/pinot_tests.yml |
Update CI to use JDK 21 and drop Java 11 test matrix entries. |
.github/workflows/pinot_multi_stage_query_engine_compatibility_tests.yml |
Update compatibility workflow to use JDK 21. |
.github/workflows/pinot_compatibility_tests.yml |
Update compatibility workflow to use JDK 21. |
Comments suppressed due to low confidence (2)
docker/images/pinot/README.md:57
- The documented docker-build.sh interface doesn’t match the actual script: README says it accepts [Docker Tag] [Git Branch] [Pinot Git URL] [Kafka Version] [Java Version], but docker-build.sh only reads 4 args (tag, git ref, kafka version, java version) and never takes a Git URL. As-is, the README examples pass the Git URL as the Kafka version argument, producing incorrect builds. Please update the README usage/examples to match the script (or update the script to accept a Git URL).
There is a docker build script which will build a given Git repo/branch and tag the image.
Usage:
```SHELL
./docker-build.sh [Docker Tag] [Git Branch] [Pinot Git URL] [Kafka Version] [Java Version]
This script will check out Pinot Repo [Pinot Git URL] on branch [Git Branch] and build the docker image for that.
The docker image is tagged as [Docker Tag].
Docker Tag: Name and tag your docker image. Default is pinot:latest.
Git Branch: The Pinot branch to build. Default is master.
Pinot Git URL: The Pinot Git Repo to build, users can set it to their own fork. Please note that, the URL is https:// based, not git://. Default is the Apache Repo: https://github.com/apache/pinot.git.
Kafka Version: The Kafka Version to build pinot with. Default is 2.0
Java Version: The Java build and runtime image version. Default is 21
- Example of building and tagging a snapshot on your own fork:
./docker-build.sh pinot_fork:snapshot-5.2 snapshot-5.2 https://github.com/your_own_fork/pinot.git- Example of building a release version:
./docker-build.sh pinot:release-0.1.0 release-0.1.0 https://github.com/apache/pinot.git**docker/images/pinot/docker-build.sh:55**
* docker-build.sh forwards KAFKA_VERSION and JAVA_VERSION as build args, but Dockerfile doesn’t consume either of these args (it uses PINOT_BASE_IMAGE_TAG and JDK_VERSION instead). This means the script’s Java/Kafka parameters (including the new default of 21) are effectively no-ops and can mislead users. Please either wire these args through to the Dockerfile (e.g., derive PINOT_BASE_IMAGE_TAG/JDK_VERSION from JAVA_VERSION, and use/propagate KAFKA_VERSION), or remove the unused parameters and align the script/README accordingly.
if [[ "$#" -gt 3 ]]
then
JAVA_VERSION=$4
else
JAVA_VERSION=21
fi
echo "Trying to build Pinot docker image on git ref: [ ${PINOT_GIT_REF} ] and tag it as: [ ${DOCKER_TAG} ]. Kafka Dependencies: [ ${KAFKA_VERSION} ]. Java Version: [ ${JAVA_VERSION} ]."
docker build --no-cache -t ${DOCKER_TAG} --build-arg PINOT_GIT_REF=${PINOT_GIT_REF} --build-arg KAFKA_VERSION=${KAFKA_VERSION} --build-arg JAVA_VERSION=${JAVA_VERSION} -f Dockerfile .
</details>
23544ba to
561faee
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 46 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
docker/images/pinot/docker-build.sh:54
- docker-build.sh passes KAFKA_VERSION and JAVA_VERSION as build args, but docker/images/pinot/Dockerfile doesn't declare or use either ARG, so these CLI parameters (and the new default JAVA_VERSION=21) have no effect on the build. Either update Dockerfile to consume these args (e.g., map JAVA_VERSION to PINOT_BASE_IMAGE_TAG/JDK_VERSION) or remove the unused parameters/echo text to avoid misleading users.
if [[ "$#" -gt 3 ]]
then
JAVA_VERSION=$4
else
JAVA_VERSION=21
fi
echo "Trying to build Pinot docker image on git ref: [ ${PINOT_GIT_REF} ] and tag it as: [ ${DOCKER_TAG} ]. Kafka Dependencies: [ ${KAFKA_VERSION} ]. Java Version: [ ${JAVA_VERSION} ]."
docker build --no-cache -t ${DOCKER_TAG} --build-arg PINOT_GIT_REF=${PINOT_GIT_REF} --build-arg KAFKA_VERSION=${KAFKA_VERSION} --build-arg JAVA_VERSION=${JAVA_VERSION} -f Dockerfile .
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 46 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
docker/images/pinot/docker-build.sh:55
- The script passes
--build-arg JAVA_VERSION=..., butdocker/images/pinot/Dockerfiledoesn’t declare/useJAVA_VERSION(it usesPINOT_BASE_IMAGE_TAGandJDK_VERSION). As a result, the Java version argument (and the new default of 21) has no effect on the image build. Align the build args with the Dockerfile (e.g., passJDK_VERSIONand/or derivePINOT_BASE_IMAGE_TAGfrom the requested version) sodocker-build.shactually controls the JDK used.
if [[ "$#" -gt 3 ]]
then
JAVA_VERSION=$4
else
JAVA_VERSION=21
fi
echo "Trying to build Pinot docker image on git ref: [ ${PINOT_GIT_REF} ] and tag it as: [ ${DOCKER_TAG} ]. Kafka Dependencies: [ ${KAFKA_VERSION} ]. Java Version: [ ${JAVA_VERSION} ]."
docker build --no-cache -t ${DOCKER_TAG} --build-arg PINOT_GIT_REF=${PINOT_GIT_REF} --build-arg KAFKA_VERSION=${KAFKA_VERSION} --build-arg JAVA_VERSION=${JAVA_VERSION} -f Dockerfile .
docker/images/pinot/README.md:43
- The
docker-build.shusage/argument documentation is inconsistent with the script implementation. The script accepts[Docker Tag] [PINOT_GIT_REF] [Kafka Version] [Java Version]and does not accept a Git URL, but this doc still describes[Git Branch] [Pinot Git URL](and the examples below will end up passing a URL as the Kafka version). Please update this section to match the actualdocker-build.shparameters (or update the script to support the documented args).
```SHELL
./docker-build.sh [Docker Tag] [Git Branch] [Pinot Git URL] [Kafka Version] [Java Version]
This script will check out Pinot Repo [Pinot Git URL] on branch [Git Branch] and build the docker image for that.
The docker image is tagged as [Docker Tag].
Docker Tag: Name and tag your docker image. Default is pinot:latest.
Git Branch: The Pinot branch to build. Default is master.
Pinot Git URL: The Pinot Git Repo to build, users can set it to their own fork. Please note that, the URL is https:// based, not git://. Default is the Apache Repo: https://github.com/apache/pinot.git.
</details>
|
I didn't have time to review this yet, but are we following the approach discussed in #15343 where the driver and external connectors (the code that runs inside flink/spark) are still in java 11? |
flink requires pinot-core and pinot-segment-local etc. So we have to ask users to upgrade to flink 2.x with jdk21 otherwise we drop the support for flink 1.x, same for Spark. |
xiangfu0
force-pushed
the
codex/raise-java-baseline-21
branch
from
26c0f7e to
c5e66af
Compare
xiangfu0
force-pushed
the
codex/raise-java-baseline-21
branch
from
c5e66af to
455ebd4
Compare
|
move the changes related to metrics stuffs from this PR to #18259 |
|
I am ok upgrading the version, but other users have raised concerns about that in that issue. I proposed #17501 as a way to keep most code in Java 21 and only few modules in older versions. Can we follow that approach? |
Yes, right now we only keep pinot-spi/pinot-segment-spi/pinot-common/pinot-*-client to JDK11 |
Document the correct 4-arg signature for docker-build.sh (drop 5th 'Git URL' arg, fix Kafka default 2.0→3.0, update examples accordingly).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
xiangfu0
force-pushed
the
codex/raise-java-baseline-21
branch
from
455ebd4 to
b4c324f
Compare
What changed
pinot-spi,pinot-segment-spi,pinot-timeseries-spi,pinot-common,pinot-java-client,pinot-jdbc-client) on Java 11 bytecodeWhy
Issue #15343 proposes moving Pinot services to Java 21 while preserving lower-bytecode compatibility for library artifacts consumed from external JVMs. The service modules need the Java 21 baseline, but the SPI and client modules still need to publish lower-target artifacts so consumers do not inherit Java 21-only jars.
Impact
Validation
./mvnw -pl pinot-query-runtime,pinot-timeseries/pinot-timeseries-planner,pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql -am spotless:apply -DskipTests./mvnw -pl pinot-query-runtime,pinot-timeseries/pinot-timeseries-planner,pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql -am checkstyle:check license:format license:check -DskipTests./mvnw -pl pinot-query-runtime,pinot-timeseries/pinot-timeseries-planner,pinot-plugins/pinot-timeseries-lang/pinot-timeseries-m3ql -am package -DskipTests -DskipShade -T1C./mvnw -pl pinot-spi,pinot-segment-spi,pinot-common,pinot-timeseries/pinot-timeseries-spi,pinot-clients/pinot-java-client,pinot-clients/pinot-jdbc-client -am spotless:apply -DskipTests./mvnw -pl pinot-spi,pinot-segment-spi,pinot-common,pinot-timeseries/pinot-timeseries-spi,pinot-clients/pinot-java-client,pinot-clients/pinot-jdbc-client -am checkstyle:check license:format license:check -DskipTests./mvnw -pl pinot-spi,pinot-segment-spi,pinot-common,pinot-timeseries/pinot-timeseries-spi,pinot-clients/pinot-java-client,pinot-clients/pinot-jdbc-client -am package -DskipTests -DskipShade -T1C