Verify the Java 11 pinned client artifacts actually run on a Java 11 JVM in CI - #19102
Merged
Merged
Conversation
yashmayya
added
testing
yashmayya
force-pushed
the
verify-java11-clients-in-ci
branch
from
July 27, 2026 22:33
6fae298 to
dccd673
Compare
| import org.apache.pinot.client.PinotClientTransport; | ||
|
|
||
|
|
||
| /** |
Contributor
There was a problem hiding this comment.
(minor) For new javadocs, use markdown style. We should also add this into the AI memory
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a dedicated CI verification that Pinot’s Java-11-pinned SPI/client artifacts (and their transitive runtime dependency closure) actually execute on a real Java 11 JVM. It does so by introducing a small “verifier” module that (1) scans the resolved classpath for too-new bytecode and (2) runs targeted runtime exercises across SPI, common, java client, and JDBC client code paths, driven by a GitHub Actions workflow that builds on JDK 25 and executes on JDK 11.
Changes:
- Add a new root-level module (
pinot-java11-client-verifier) containing a runnable Java 11 verifier plus unit tests for the classpath-closure scanner. - Add a new GitHub Actions workflow and driver script to build on JDK 25 and run the verifier on Java 11 using the resolved runtime classpath.
- Wire the verifier module into the reactor via the root
pom.xml.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Adds the new pinot-java11-client-verifier module to the Maven reactor. |
pinot-java11-client-verifier/pom.xml |
Defines the verifier module, pins its compilation to --release 11, and writes a runtime classpath file via maven-dependency-plugin. |
pinot-java11-client-verifier/src/main/java/org/apache/pinot/java11/Java11CompatibilityVerifier.java |
Main executable verifier: JVM-version guard, closure scan, and runtime exercises for key dependency surfaces. |
pinot-java11-client-verifier/src/main/java/org/apache/pinot/java11/ClasspathClosureScanner.java |
Scans classpath entries (jars/dirs) for class-file major versions above the target JVM’s supported level, with multi-release filtering. |
pinot-java11-client-verifier/src/main/java/org/apache/pinot/java11/CannedResponseTransport.java |
Test transport to drive real client/JDBC code paths without needing a live cluster. |
pinot-java11-client-verifier/src/main/resources/sample-broker-response.json |
Fixture used to exercise response parsing and JDBC result handling. |
pinot-java11-client-verifier/src/test/java/org/apache/pinot/java11/ClasspathClosureScannerTest.java |
Unit tests that pin scanner behavior (multi-release handling, violation reporting, corrupt zip handling, etc.). |
.github/workflows/scripts/.pinot_java11_client_compat.sh |
Builds the verifier+deps on the build JDK, then runs the verifier under Java 11 with the resolved runtime classpath. |
.github/workflows/pinot_java11_client_compatibility.yml |
New CI workflow that installs Java 11 + Java 25 and runs the verifier in PRs and master pushes. |
Comment on lines
+623
to
+633
| BrokerGrpcQueryClient client = null; | ||
| try { | ||
| client = new BrokerGrpcQueryClient("localhost", 8010, new GrpcConfig(Map.of())); | ||
| require(!client.getChannel().isShutdown(), "the gRPC channel was already shut down on creation"); | ||
| } finally { | ||
| if (client != null) { | ||
| client.close(); | ||
| } | ||
| } | ||
| require(client.getChannel().isTerminated(), "the gRPC channel did not terminate on close"); | ||
| } |
Comment on lines
+270
to
+287
| String effectiveName = name; | ||
| if (name.startsWith(MULTI_RELEASE_PREFIX)) { | ||
| int versionEnd = name.indexOf('/', MULTI_RELEASE_PREFIX.length()); | ||
| if (versionEnd < 0) { | ||
| return false; | ||
| } | ||
| int version; | ||
| try { | ||
| version = Integer.parseInt(name.substring(MULTI_RELEASE_PREFIX.length(), versionEnd)); | ||
| } catch (NumberFormatException e) { | ||
| // Not a well-formed multi-release directory; treat it as an inert resource. | ||
| return false; | ||
| } | ||
| if (version > targetJavaFeatureVersion) { | ||
| return false; | ||
| } | ||
| effectiveName = name.substring(versionEnd + 1); | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19102 +/- ##
=========================================
Coverage 65.50% 65.51%
Complexity 1423 1423
=========================================
Files 3430 3430
Lines 218058 218010 -48
Branches 34651 34648 -3
=========================================
- Hits 142835 142822 -13
+ Misses 63654 63625 -29
+ Partials 11569 11563 -6
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:
|
xiangfu0
approved these changes
Jul 28, 2026
yashmayya
force-pushed
the
verify-java11-clients-in-ci
branch
from
July 28, 2026 19:19
dccd673 to
48960c6
Compare
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.
What this closes
Six modules deliberately compile to Java 11 bytecode with a hard-coded compiler
release(not inherited from${jdk.version}), so that third-party plugins built againstpinot-spiand applications embedding the Java/JDBC client are not forced onto JDK 25:pinot-spi,pinot-segment-spi,pinot-timeseries/pinot-timeseries-spi,pinot-common,pinot-clients/pinot-java-client,pinot-clients/pinot-jdbc-client.--release 11already guarantees Pinot's own code in those modules is Java-11-clean — it restricts against the Java 11 API signature set, not just the bytecode version. The gap is the transitive dependency closure, which nothing verifies. Today it happens to hold (arrow-vector 19.0.0 is major 55, calcite-core 1.42.0 is major 52, helix-core 2.0.1 is major 55), but any routine dependency bump could ship a Java 17+ jar into the client's closure and nobody would notice until a user on Java 11 reportsUnsupportedClassVersionErrororNoSuchMethodError. Every existing matrix in.github/workflows/isjava: [ 25 ]; no job anywhere loads these artifacts on a Java 11 JVM.A job that only inspected class-file major versions of Pinot's own jars would be vacuous, so this one executes the clients on a real Java 11 JVM.
Shape
The build cannot run on Java 11 — the root pom enforces
requireJavaVersion [25,), and Pinot's services have a genuine Java 25 floor (datasketches-java9.0.0 is major 69 and the code usesjava.lang.foreign.MemorySegment, see 2892236 / #19014).-Djdk.version=11does not help; that knob only lowers the compile target while the enforcer checks the running JDK. So the job is necessarily two-JDK: build with JDK 25, then run the verification under Java 11..github/workflows/pinot_java11_client_compatibility.yml— installs Java 11 first and JDK 25 second (soJAVA_HOMEis the build JDK), and passes the Java 11 path viasteps.java11.outputs.path. I confirmedpathis a real output ofactions/setup-java@v5rather than assuming it; the script additionally falls back toJAVA_HOME_11_X64/JAVA_HOME_11_ARM64so it does not depend on the runner architecture..github/workflows/scripts/.pinot_java11_client_compat.sh— resolves the Java 11 JVM, builds-pl pinot-java11-client-verifier -am, then launches the verifier with the resolved runtime closure.pinot-java11-client-verifier/— a new module at the reactor root, alongside the existingpinot-compatibility-verifierandpinot-dependency-verifier(both of which are root modules, and the former has exactly this shape: a runnablesrc/main/javaverifier driven by a script in.github/workflows/scripts/). Compiled atrelease 11itself, since the script launches its classes on the Java 11 JVM.maven.deploy.skipkeeps a CI harness out of the published artifacts, and nothing in the reactor depends on it.The module depends on both clients, which makes its resolved runtime closure the union of the two consumer-facing closures.
maven-dependency-plugin:build-classpathwrites that closure totarget/runtime-classpath.txtatprepare-package, and the script hands exactly that tojava -cp.What the 18 checks do
Closure scan (
ClasspathClosureScanner) — walks all ~220 classpath entries and fails on any class file a Java 11 JVM could not load. This is a legitimate complement to--release, not a duplicate: it covers third-party jars, which--releasesays nothing about. Two categories are correctly not violations, because a Java 11 JVM never loads them:module-info.classdescriptors, at any version.META-INF/versions/<n>/entries wheren> the target release.The multi-release handling is load-bearing, not defensive:
jackson-core-2.22.1ships major 61 underMETA-INF/versions/17/and major 65 underversions/21/, andjersey-common-2.48ships major 65 — without it the job would fail on a dependency every Pinot consumer has.Runtime exercises — these construct and use the artifacts, pulling in Jackson, Arrow, Calcite, Helix, protobuf, gRPC/Netty, async-http-client and the JNI compression codecs:
spi-schema-deserialization,spi-table-config-deserializationsegment-spi-data-bufferPinotDataBuffer—Unsafe/sun.misc.Cleanerreflection, which the class-file scan cannot seetimeseries-spi-plan-serdeTimeSeriesPlanSerderound trip,TimeBucketscommon-data-schema-round-trip,common-broker-response-deserializationDataSchemabinary + JSON,BrokerResponseNativeover a realistic broker responsecommon-response-encoderscommon-calcite-sql-parsingCalciteSqlParseron a group-by/order-by/limit query plus aSEToptioncommon-helix-segment-metadataSegmentZKMetadata↔ZNRecordviaZNRecordSerializer,ExternalViewcommon-grpc-response-decodingBrokerResponse→ every compression codec → every encoder, exactly asGrpcConnectionunpacks a server responsecommon-grpc-channel-constructionBrokerGrpcQueryClient→ Netty channel + pooled direct-buffer allocatorjava-client-http-transportjava-client-query-execution,java-client-prepared-statementConnectionFactory→Connection.execute→ResultSetGroupvalues +ExecutionStats, against a canned transportjdbc-driver-registrationDriverManagerauto-discovery throughMETA-INF/services/java.sql.DriverandServiceLoader, with no explicitClass.forNamejdbc-result-setPinotResultSetrows,ResultSetMetaData, SQL type mappingNo live cluster, per the tradeoff below.
Vacuity guards. A green job that cannot go red is worse than no job, so three things are asserted before any of the above counts:
Runtime.version().feature()), because running on the build JDK would make every other check pass for the wrong reason. Failing this aborts the run rather than printing 17 meaningless passes.target/classeswould satisfy the guard on its own and make it unfalsifiable.optional/providedshrinks coverage loudly instead of silently.Proving it can fail
Run locally against Temurin 11.0.22. Baseline: 18/18 pass, exit 0,
major versions: {46=236, 47=1778, 48=104, 49=3104, 50=4391, 51=1612, 52=52314, 53=15, 55=5926}— no bucket above 55, and the counts make it evident the scan really looked.1. Third-party dependency regression (the actual threat). Added
org.apache.datasketches:datasketches-java— already at 9.0.0 independencyManagement, and major 69 precisely because #19014 moved it to Java 25 — topinot-java-client. This is the realistic shape: someone adds a sketch-backed client helper and silently breaks every Java 11 consumer.Script exit 1.
2. Post-Java-11 API with the pin in place. Added
Arrays.stream(_columnNames).toList()(Java 16+) toDataSchema.toString().--release 11rejects it at compile time:This is the pre-existing guarantee, and it is exactly why the closure scan is a complement rather than a duplicate — this failure mode cannot reach runtime while the pin is in place.
3. Pin removed — proving the execution checks go red, not just the scan. Kept the
toList()call and changedpinot-commontorelease 17. It now compiles, and pinot-common ships major 61:Eight exercise checks fail by name, so the harness catches a real load failure and not merely a jar inspection.
4. Mis-wired JDK. Same classpath on JDK 25:
Exit 1. (Arrow also fails on 25 without
--add-opens, which is consistent with the deliberate absence of those flags — see below.)5. Coverage silently shrinking. Swapped the
pinot-timeseries-spijar for an identically-contented file under a different name, so its classes still load but the artifact is unrecognisable:All injections reverted; the tree is clean and the baseline run is green again.
ClasspathClosureScannerTest(14 tests, runs in the normal unit-test job — no Java 11 JVM needed) pins the scanner's fail path, since both of its filters fail open and a regression there would produce a permanently green job rather than a red one. It covers: major 65 at the jar root → violation;module-info.classat 65 → ignored;META-INF/versions/17|21/above target → ignored;versions/9/at or below target → violation; malformed version directory → ignored; missing0xCAFEBABEand truncated entries → not counted; archive vs directory counters kept apart; corrupt zip →IOException; reporting capped while the total count stays exact; target version honoured at 11/17/21.Notes on two deliberate choices
No live cluster. A cross-JVM integration test (cluster on JDK 25, client on Java 11) would be higher fidelity, but much slower and flakier, and it would put a multi-process cluster on the critical path of what should be a fast signal on every PR. The canned-transport approach still drives the real
Connection/ResultSetGroup/PinotResultSetcode and the real deserialization paths, which is where a bad bump surfaces. If someone wants the full-fidelity version later it belongs in a separate opt-in job, not as a dependency of this smoke.No
--add-opens/-Dio.netty.tryReflectionSetAccessible=true. Those flags exist in Pinot's launch scripts for JDK 17+; adding them here would paper over the runtime breakage this job exists to catch. Verified empirically that nothing on the closure needs them on Java 11 — Arrow logs an illegal-reflective-access warning and works.-Dshade.phase.prop=noneon the build is worth a note:pinot-java-clientandpinot-jdbc-clienteach produce a ~148 MB shaded jar, andshadedArtifactAttached=truemeans those jars never appear on the classpath this job verifies. Without the flag the job spends minutes building 296 MB it does not use and then pushes it into~/.m2, whichactions/cacheuploads under a key the other Maven jobs share.-DskipShade=trueis not sufficient — it only deactivates thepinot-jdbc-clientprofile, whilepinot-java-clientsetsshade.phase.prop=packageunconditionally. Both verified by inspecting the produced jars.Follow-ups, deliberately not in this PR
Found while analysing this area; left out to keep the change to one concern:
.github/workflows/build-pinot-base-docker-image.yml:32and:63still build base images forjdk_version: [21, 25].pinot-base-build:21-*can no longer build master (the enforcer rejects Java 21) andpinot-base-runtime:21-*would hit the datasketches Java 25 floor at runtime.docker/images/pinot-base/pinot-base-build/*.dockerfileand.../pinot-base-runtime/*.dockerfilealso still defaultARG JAVA_VERSION=21.workflow_dispatch-only, so latent rather than actively broken.pinot-tools/src/main/resources/appAssemblerScriptTemplate:204guards the--add-opensflags behindif [ "$(jdk_version)" -gt 11 ], which can never be false now.extra-enforcer-rules'enforceBytecodeVersionand fire on every./mvnw install. That is a project-wide dependency-policy decision, and keeping it next to the exercise checks means one place reports on the whole closure.Release notes
none