chore: no-ticket: bump hadoop-common, add testing#7919
Merged
devinrsmith merged 3 commits intodeephaven:mainfrom Apr 20, 2026
Merged
chore: no-ticket: bump hadoop-common, add testing#7919devinrsmith merged 3 commits intodeephaven:mainfrom
devinrsmith merged 3 commits intodeephaven:mainfrom
Conversation
This bumps hadoop-common from 3.4.1 to 3.4.3 and adds explicit testing to prove hadoop Configuration "works" and classpath testing to prove that without the dependency, hadoop Configuration is "broken". As part of this bump, the transitive dependencies are as follows: * `commons-collections:commons-collections` was changed to `org.apache.commons:commons-collections4` * `hadoop-shaded-guava` was bumped from 1.3.0 to 1.5.0 * `woodstox-core` was left unchanged (still newer than hadoop-common's version)
Contributor
No docs changes detected for 6d0675f |
devinrsmith
added
NoDocumentationNeeded
NoReleaseNotesNeeded
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the build to use org.apache.hadoop:hadoop-common 3.4.3 and adds a dedicated test project to validate the explicitly-managed runtime dependencies needed for org.apache.hadoop.conf.Configuration to function.
Changes:
- Bump Hadoop version and update the inlined transitive dependency coordinates (
commons-collections4,hadoop-shaded-guava). - Add a new
extensions-hadoop-common-testGradle project with positive and “missing dependency” test suites. - Update
io.deephaven.hadoop-common-dependenciesto reflect the new dependency set and document the justification.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Includes the new :extensions-hadoop-common-test project in the build. |
| gradle/libs.versions.toml | Bumps hadoop to 3.4.3 and updates the related dependency aliases/versions. |
| extensions/hadoop-common-test/build.gradle | Defines multiple JVM test suites to validate required/missing runtime deps for Hadoop Configuration. |
| extensions/hadoop-common-test/gradle.properties | Registers the new module as JAVA_LOCAL. |
| extensions/hadoop-common-test/src/test/java/io/deephaven/HadoopCommonDependenciesTest.java | Smoke-tests Configuration construction and basic get/set with the explicit runtime deps present. |
| extensions/hadoop-common-test/src/missingWoodstoxTest/java/io/deephaven/MissingWoodstoxTest.java | Verifies Configuration fails when Woodstox is absent. |
| extensions/hadoop-common-test/src/missingCollections4Test/java/io/deephaven/MissingCollections4Test.java | Verifies Configuration fails when commons-collections4 is absent. |
| extensions/hadoop-common-test/src/missingHadoopShadedGuavaTest/java/io/deephaven/MissingHadoopShadedGuavaTest.java | Verifies Configuration behavior when hadoop-shaded-guava is absent (currently has correctness issues). |
| buildSrc/src/main/groovy/io.deephaven.hadoop-common-dependencies.gradle | Updates the explicitly-managed runtime dependencies to use commons-collections4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+24
| // We can't test both getProperty and setProperty in the same JVM given the stateful nature classloading errors. | ||
| // We _could_ create a separate class and use `forkEvery = 1`, but that is extra test orchestration that doesn't add | ||
| // much value. We'll choose to randomly test one of them for any given test run. (This could theoretically cause | ||
| // issues if something downstream alerted about tests that flapped from "SUCCESS" to "IGNORED", but we don't have | ||
| // anything like that right now.) | ||
| private static final boolean TEST_GET_PROPERTY = ThreadLocalRandom.current().nextBoolean(); | ||
| private static final boolean TEST_SET_PROPERTY = !TEST_GET_PROPERTY; | ||
|
|
There was a problem hiding this comment.
Using ThreadLocalRandom to decide which assertion runs makes this test non-deterministic and can mask regressions where only one of get/set starts requiring (or stops requiring) the shaded-guava classes. Prefer making both checks deterministic by isolating them into separate JVM forks (e.g., separate test tasks/suites or configuring the test task to fork per test class) instead of random selection + assumptions.
cpwright
approved these changes
Apr 20, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This bumps hadoop-common from 3.4.1 to 3.4.3 and adds explicit testing to prove hadoop Configuration "works" and classpath testing to prove that without the dependency, hadoop Configuration is "broken".
As part of this bump, the transitive dependencies are as follows:
commons-collections:commons-collectionswas changed toorg.apache.commons:commons-collections4hadoop-shaded-guavawas bumped from 1.3.0 to 1.5.0woodstox-corewas left unchanged (still newer than hadoop-common's version)