Bypass Dependabot for ClassLoaderTest's pinned commons-io 2.11.0 fixture#18335
Merged
xiangfu0 merged 2 commits intoApr 26, 2026
Merged
Conversation
ClassLoaderTest pins commons-io 2.11.0 as a test fixture (asserts the literal jar filename and contents to validate plugin realm isolation). The same coordinate is consumed at a newer version in the top-level pom.xml, so a repo-wide Dependabot ignore rule is not viable -- it would block legitimate commons-io upgrades. With the previous <artifactItem> declaration, Dependabot kept opening PRs that bumped the pinned 2.11.0 reference (e.g. apache#18331), forcing manual reverts. Refactor the fixture fetch to a two-step pipeline that hides the coordinate from Dependabot's Maven updater while preserving Maven's normal resolution semantics: - maven-dependency-plugin:get with <artifact>commons-io:commons-io:2.11.0</artifact> resolves the jar through the standard resolution chain (local cache, configured mirrors, authenticated repos, SHA-1 checksum validation). Dependabot scans structured groupId+artifactId+version triples in <dependency>/<plugin>/<artifactItem> blocks, not single-string <artifact> values in plugin configuration. - maven-antrun-plugin:run copies the resolved jar from ${settings.localRepository} into target/test-classes, preserving the literal filename commons-io-2.11.0.jar that ClassLoaderTest.assemblyBasedRealm asserts on. Also update .github/dependabot.yml to drop the now-obsolete caveat about commons-io. Verified locally: ClassLoaderTest's 5 cases (assemblyBasedRealm, classRealms, classicPluginClassloader, limitedPluginRealm, unlimitedPluginRealm) all pass on a clean target dir with the local commons-io 2.11.0 cache deleted (forcing fresh resolution). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xiangfu0
added
buildAndPackage
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18335 +/- ##
============================================
+ Coverage 63.38% 63.45% +0.07%
- Complexity 1658 1668 +10
============================================
Files 3252 3252
Lines 198637 198661 +24
Branches 30765 30770 +5
============================================
+ Hits 125897 126056 +159
+ Misses 62667 62509 -158
- Partials 10073 10096 +23
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:
|
Jackie-Jiang
approved these changes
Apr 26, 2026
Contributor
There was a problem hiding this comment.
Consider modifying all of the pinned version
Contributor
Author
There was a problem hiding this comment.
Done in 074e944. All five pinned ClassLoaderTest fixtures (pinot-dropwizard, pinot-yammer:shaded, pinot-yammer for the unpack, metrics-core, plus the previously-handled commons-io) now go through the same dependency:get + antrun copy/unzip pipeline. The <artifactItem> blocks and the >= <next> ignore rules in .github/dependabot.yml are gone. Verified locally with a wiped local cache for all four fixture coordinates — all 5 ClassLoaderTest cases pass.
…ures Address review feedback on apache#18335: extend the dependency:get + antrun copy/unzip pattern to every pinned ClassLoaderTest fixture, not just commons-io. Before: pinot-dropwizard, pinot-yammer (x2 shaded), and metrics-core were still declared as <artifactItem> blocks under copy-pinot-plugins and unpack-pinot-plugins, with `versions: [">= <next>"]` ignore rules in .github/dependabot.yml suppressing Dependabot bumps. Those rules worked because none of those coordinates were used elsewhere -- but the asymmetric handling left a footgun: any future use of one of those coordinates at a different version elsewhere in the repo would silently get blocked from updates by the repo-wide ignore rule. After: all five fixtures (including the previously-handled commons-io) are routed through the same pipeline: - maven-dependency-plugin:get with single-string <artifact> parameters resolves each unique GAV+classifier combination through Maven's normal chain (cache, mirrors, authenticated repos, SHA-1 validation). Dependabot's Maven updater does not scan single-string <artifact> values, so the pinned coordinates are invisible to it. - A single maven-antrun-plugin:run execution stages the resolved jars from ${settings.localRepository} into target/test-classes/plugins/ with the literal filenames the test asserts on, and unzips the non-shaded pinot-yammer 0.10.0 into the assemblybased-pinot-plugin classes/ subdirectory. The .github/dependabot.yml fixture-specific ignore rules are removed since no <artifactItem> blocks remain for these coordinates. Verified locally: ClassLoaderTest's 5 cases pass on a fully empty target dir AND empty local cache for all four fixture coordinates, confirming the resolve+stage pipeline is idempotent and self-bootstrapping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xiangfu0
added a commit
to xiangfu0/pinot
that referenced
this pull request
Apr 27, 2026
…al classpath guard After rebase onto upstream/master (fixes the CRITICAL "branch reverts unrelated master commits" finding by absorbing apache#18335, apache#18285, apache#18341, apache#18340), close the remaining MAJOR review gaps: 1. PinotDataSource classpath conflict guard, both connectors: - Treat LinkageError as a conflict, not as "v3 absent". A linkage failure means the v3 class IS resolvable to the loader but cannot run — the case the guard exists for. Falling back to "absent" would let the non-deterministic-resolution failure mode the guard is supposed to prevent slip through. Bias to fail-closed; users who genuinely need both jars can still set -Dpinot.spark.connector.skip-conflict-guard=true. - Add a symmetric guard to pinot-spark-3-connector that probes for the v4 PinotDataSource class. The conflict can now be caught regardless of which connector Spark instantiates first. 2. PinotWriteBuilder (spark-3): keep a deprecated 2-arg constructor `PinotWriteBuilder(filters: Array[Filter], logicalWriteInfo: LogicalWriteInfo)` so the constructor-signature change does not silently break external embedders mid-deprecation. The `filters` parameter is ignored at build time and overwrite()/truncate() still throw — purely a binary-compat shim during the spark-3 sunset window. 3. pinot-batch-ingestion-spark-4 netty.version override: expand the property comment to call out that the override propagates into the production shaded jar (not just the test classpath), document validated spark4.version pairings, and add SparkVersionAlignmentTest that pins netty 4.2.x on the test classpath via Class.forName(KQueueIoHandler) + getImplementationVersion. A future spark4.version bump that shadows netty back to 4.1 (or forward to 4.3) now fails the build immediately. 4. FilterPushDownTest (both connectors): add an explicit regression test for `In(attr, Array.empty)` — the empty-IN compile path was previously covered only by the umbrella test and would not have surfaced a regression cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xiangfu0
added a commit
to xiangfu0/pinot
that referenced
this pull request
Apr 27, 2026
…al classpath guard After rebase onto upstream/master (fixes the CRITICAL "branch reverts unrelated master commits" finding by absorbing apache#18335, apache#18285, apache#18341, apache#18340), close the remaining MAJOR review gaps: 1. PinotDataSource classpath conflict guard, both connectors: - Treat LinkageError as a conflict, not as "v3 absent". A linkage failure means the v3 class IS resolvable to the loader but cannot run — the case the guard exists for. Falling back to "absent" would let the non-deterministic-resolution failure mode the guard is supposed to prevent slip through. Bias to fail-closed; users who genuinely need both jars can still set -Dpinot.spark.connector.skip-conflict-guard=true. - Add a symmetric guard to pinot-spark-3-connector that probes for the v4 PinotDataSource class. The conflict can now be caught regardless of which connector Spark instantiates first. 2. PinotWriteBuilder (spark-3): keep a deprecated 2-arg constructor `PinotWriteBuilder(filters: Array[Filter], logicalWriteInfo: LogicalWriteInfo)` so the constructor-signature change does not silently break external embedders mid-deprecation. The `filters` parameter is ignored at build time and overwrite()/truncate() still throw — purely a binary-compat shim during the spark-3 sunset window. 3. pinot-batch-ingestion-spark-4 netty.version override: expand the property comment to call out that the override propagates into the production shaded jar (not just the test classpath), document validated spark4.version pairings, and add SparkVersionAlignmentTest that pins netty 4.2.x on the test classpath via Class.forName(KQueueIoHandler) + getImplementationVersion. A future spark4.version bump that shadows netty back to 4.1 (or forward to 4.3) now fails the build immediately. 4. FilterPushDownTest (both connectors): add an explicit regression test for `In(attr, Array.empty)` — the empty-IN compile path was previously covered only by the umbrella test and would not have surfaced a regression cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xiangfu0
added a commit
to xiangfu0/pinot
that referenced
this pull request
May 1, 2026
…al classpath guard After rebase onto upstream/master (fixes the CRITICAL "branch reverts unrelated master commits" finding by absorbing apache#18335, apache#18285, apache#18341, apache#18340), close the remaining MAJOR review gaps: 1. PinotDataSource classpath conflict guard, both connectors: - Treat LinkageError as a conflict, not as "v3 absent". A linkage failure means the v3 class IS resolvable to the loader but cannot run — the case the guard exists for. Falling back to "absent" would let the non-deterministic-resolution failure mode the guard is supposed to prevent slip through. Bias to fail-closed; users who genuinely need both jars can still set -Dpinot.spark.connector.skip-conflict-guard=true. - Add a symmetric guard to pinot-spark-3-connector that probes for the v4 PinotDataSource class. The conflict can now be caught regardless of which connector Spark instantiates first. 2. PinotWriteBuilder (spark-3): keep a deprecated 2-arg constructor `PinotWriteBuilder(filters: Array[Filter], logicalWriteInfo: LogicalWriteInfo)` so the constructor-signature change does not silently break external embedders mid-deprecation. The `filters` parameter is ignored at build time and overwrite()/truncate() still throw — purely a binary-compat shim during the spark-3 sunset window. 3. pinot-batch-ingestion-spark-4 netty.version override: expand the property comment to call out that the override propagates into the production shaded jar (not just the test classpath), document validated spark4.version pairings, and add SparkVersionAlignmentTest that pins netty 4.2.x on the test classpath via Class.forName(KQueueIoHandler) + getImplementationVersion. A future spark4.version bump that shadows netty back to 4.1 (or forward to 4.3) now fails the build immediately. 4. FilterPushDownTest (both connectors): add an explicit regression test for `In(attr, Array.empty)` — the empty-IN compile path was previously covered only by the umbrella test and would not have surfaced a regression cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xiangfu0
added a commit
to xiangfu0/pinot
that referenced
this pull request
May 7, 2026
…al classpath guard After rebase onto upstream/master (fixes the CRITICAL "branch reverts unrelated master commits" finding by absorbing apache#18335, apache#18285, apache#18341, apache#18340), close the remaining MAJOR review gaps: 1. PinotDataSource classpath conflict guard, both connectors: - Treat LinkageError as a conflict, not as "v3 absent". A linkage failure means the v3 class IS resolvable to the loader but cannot run — the case the guard exists for. Falling back to "absent" would let the non-deterministic-resolution failure mode the guard is supposed to prevent slip through. Bias to fail-closed; users who genuinely need both jars can still set -Dpinot.spark.connector.skip-conflict-guard=true. - Add a symmetric guard to pinot-spark-3-connector that probes for the v4 PinotDataSource class. The conflict can now be caught regardless of which connector Spark instantiates first. 2. PinotWriteBuilder (spark-3): keep a deprecated 2-arg constructor `PinotWriteBuilder(filters: Array[Filter], logicalWriteInfo: LogicalWriteInfo)` so the constructor-signature change does not silently break external embedders mid-deprecation. The `filters` parameter is ignored at build time and overwrite()/truncate() still throw — purely a binary-compat shim during the spark-3 sunset window. 3. pinot-batch-ingestion-spark-4 netty.version override: expand the property comment to call out that the override propagates into the production shaded jar (not just the test classpath), document validated spark4.version pairings, and add SparkVersionAlignmentTest that pins netty 4.2.x on the test classpath via Class.forName(KQueueIoHandler) + getImplementationVersion. A future spark4.version bump that shadows netty back to 4.1 (or forward to 4.3) now fails the build immediately. 4. FilterPushDownTest (both connectors): add an explicit regression test for `In(attr, Array.empty)` — the empty-IN compile path was previously covered only by the umbrella test and would not have surfaced a regression cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
3 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.
Summary
ClassLoaderTestpinscommons-io 2.11.0as a test fixture — it asserts the literal jar filenamecommons-io-2.11.0.jarand depends on that exact version's contents to validate plugin realm isolation. The same coordinate is also legitimately consumed at a newer version in the top-level pom.xml, so a repo-wide Dependabot ignore rule is not viable: it would block real commons-io upgrades.With the previous
<artifactItem>declaration in pinot-spi/pom.xml, Dependabot kept opening PRs that bumped the pinned 2.11.0 reference (e.g. #18331, opened the day after #18308 merged the legitimatecommons-io.versionupgrade in root pom). Each PR required manual revert of the pinot-spi fixture portion. This change makes Dependabot stop scanning the pinned reference altogether.Approach
Refactor the fixture fetch to a two-step pipeline that hides the coordinate from Dependabot's Maven updater while preserving Maven's normal resolution semantics:
maven-dependency-plugin:getwith<artifact>commons-io:commons-io:2.11.0</artifact>— resolves the jar through Maven's standard resolution chain (local cache → configured mirrors → authenticated repos → SHA-1 checksum validation). Dependabot's Maven updater scans structured<groupId>/<artifactId>/<version>triples inside<dependency>/<plugin>/<artifactItem>blocks; it does not parse single-string<artifact>values in plugin configuration. Wide range of ASF projects (Calcite, Drill, etc.) use this same pattern for similar test fixtures.maven-antrun-plugin:runcopies the resolved jar from${settings.localRepository}/commons-io/commons-io/2.11.0/commons-io-2.11.0.jarintotarget/test-classes/plugins/assemblybased-pinot-plugin/, preserving the literal filename thatClassLoaderTest.assemblyBasedRealmasserts on viaCodeSource.getLocation().getPath().endsWith("commons-io-2.11.0.jar").Why not simpler alternatives:
<get src="https://repo1.maven.org/...">): bypasses local cache, configured mirrors, authenticated proxies, and SHA-1 validation. Breaks offline / mirrored / air-gapped builds.<dependency:get>with<destination>: the parameter was removed in modernmaven-dependency-plugin—dependency:getonly writes to local repo now.commons-ioignore in dependabot.yml: would block legitimate commons-io upgrades for the production version in root pom.xml.Also updates .github/dependabot.yml comment to drop the now-obsolete caveat about commons-io.
Test plan
./mvnw -pl pinot-spi -am -Dtest=ClassLoaderTest test— all 5 ClassLoaderTest cases pass (assemblyBasedRealm,classRealms,classicPluginClassloader,limitedPluginRealm,unlimitedPluginRealm).~/.m2/repository/commons-io/commons-io/2.11.0/andpinot-spi/target/, then re-ran the test —dependency:getre-resolved the jar through Maven, antrun copied it, all tests pass../mvnw -pl pinot-spi spotless:apply checkstyle:check license:format license:check— all clean.<get src="https://...">approach was rejected for bypassing Maven resolution / checksum verification; revised approach approved).🤖 Generated with Claude Code