fix(build): publish dependency-reduced POMs for shaded bundles - #19433
fix(build): publish dependency-reduced POMs for shaded bundles#19433rangareddy wants to merge 2 commits into
Conversation
Seven bundles set createDependencyReducedPom=false while also configuring dependencyReducedPomLocation, so the POM published to Maven Central still declared the artifacts that were shaded into the jar. Consumers therefore inherited those artifacts as transitive dependencies even though the classes were already inside the bundle (and relocated, so the transitive copies are not the ones the bundle uses). For hudi-aws-bundle the published POM listed hudi-aws, hudi-hive-sync, httpclient and httpcore, all of which are in the bundle's artifactSet. Enabling the reduced POM drops exactly those and keeps hudi-common and parquet-avro, which are not bundled and are genuinely required at runtime. Closes apache#16407
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR flips createDependencyReducedPom from false to true for seven shaded bundles so the POMs published to Maven Central no longer declare artifacts that are already shaded (and relocated) into the jars. The change is consistent with the bundles that already default to shade's behavior, and the reduced-POM output location was already configured under target/ and gitignored. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19433 +/- ##
============================================
+ Coverage 75.56% 75.65% +0.09%
- Complexity 32651 32789 +138
============================================
Files 2574 2576 +2
Lines 142995 143558 +563
Branches 17530 17776 +246
============================================
+ Hits 108051 108611 +560
- Misses 26908 26910 +2
- Partials 8036 8037 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
Reviewed the seven flips against each bundle's shade config, the inherited root config, and the already-built jars. The motivation is right and the consistency argument holds (hudi-timeline-server-bundle is already true, the other nine inherit shade's default of true, and all seven already point dependencyReducedPomLocation under target/).
Two things I think need to change before this lands:
hudi-hadoop-mr-bundle's reduced POM comes out with zero dependencies, andhudi-presto-bundlereacheshudi-hadoop-mr/hudi-hadoop-commononly through it. In any build where the mr-bundle is resolved from a repository rather than the reactor, the presto bundle silently loses those classes.promoteTransitiveDependenciesdefaults to false, so shade drops the direct dependency entry and everything that only arrived through it, including artifacts that are not in the jar. That is a bigger consumer-facing change than the impact section describes.
Details inline.
Suggested path: split it. Land the five with no in-repo consumers (aws, azure, datahub-sync, gcp, hive-sync) with promoteTransitiveDependencies=true, and do hadoop-mr + presto in a follow-up that first makes presto's shade inputs explicit. Your offer to stage per bundle is the right instinct; the hudi-hadoop-mr-bundle -> hudi-presto-bundle edge is why it matters.
What I checked: static analysis of the POMs plus the inherited root shade config, ShadeMojo 3.5.3 and Maven 3.9.14 bytecode for the reactor-vs-repository resolution question, contents of the already-built hudi-presto-bundle and hudi-hadoop-mr-bundle jars, and every -pl packaging/... invocation in .github/workflows and scripts/. I did not run a full Maven build, so the presto repro below is predicted from the dependency graph rather than observed.
| </relocation> | ||
| </relocations> | ||
| <createDependencyReducedPom>false</createDependencyReducedPom> | ||
| <createDependencyReducedPom>true</createDependencyReducedPom> |
There was a problem hiding this comment.
promoteTransitiveDependencies is not set anywhere in the repo, so it defaults to false. That means shade removes the direct dependency entry and everything that only arrived through it, including artifacts that are not in the bundle. Not just "artifacts it actually bundled", as the description puts it.
Here that is hudi-hive-sync, which today supplies hadoop-common, hadoop-client, hadoop-hdfs, hadoop-auth, hive-jdbc, hive-metastore and hive-common at compile scope. None of them are in this bundle's artifactSet, and the jar still contains the org.apache.hudi.hive.* classes that call into them. After this change the published POM advertises neither the classes' host jar nor its dependencies.
That may well be the right end state for a fat bundle meant to land on a Hadoop/Hive classpath, but it is a larger change than the impact section describes. Either set promoteTransitiveDependencies=true, so the reduced POM keeps the non-bundled transitives and preserves the runtime contract exactly minus what was absorbed, or call the drop out explicitly.
(The two kept deps are right: hudi-common and parquet-avro are genuinely not bundled.)
There was a problem hiding this comment.
Agreed on the mechanism, and my Impact wording was wrong — "artifacts it actually bundled" is not what promoteTransitiveDependencies=false does. Fixed in ed3ccf5 by taking your first option, promoteTransitiveDependencies=true, on all seven bundles, so the reduced POM keeps the runtime contract minus what was absorbed. Effect is visible on hudi-hadoop-mr-bundle, whose reduced POM goes from 0 to 79 dependencies (orc-core, rocksdbjni, jetty, metrics, simpleclient, lz4-java, disruptor, jaxb-api, …). The other bundles land at 74–191.
One correction on the specific example, though, because it changed what I expected to see. Those seven artifacts are not in hudi-aws-bundle's dependency closure, before or after this PR. They are provided in the root pom's dependencyManagement:
hadoop-client, hadoop-common, hadoop-hdfs, hadoop-auth,
hive-jdbc, hive-metastore, hive-common -> <scope>provided</scope>
provided is not transitive, so they never propagate from hudi-hive-sync to this bundle. mvn dependency:list -pl packaging/hudi-aws-bundle returns exactly one org.apache.hive/org.apache.hadoop artifact in the whole tree — hive-storage-api:2.6.0:compile — and no hadoop-* at all. So removing the hudi-hive-sync entry from the published POM was not dropping them; they were never advertised to consumers in the first place.
They are compile scope where you looked, in hudi-sync/hudi-hive-sync/pom.xml, which is what I assume produced the list. The root-level management is what makes them non-transitive one level up.
That does not change the outcome here: your general point was correct, non-bundled transitives were being dropped, and promoteTransitiveDependencies=true now preserves them. It only means the classes in this bundle that call into Hadoop/Hive were relying on a provided classpath before this PR as well, which is the normal contract for a bundle meant to land on a Hadoop/Hive cluster. Say the word if you would rather I also called that out in the Impact section — I have rewritten it, but I did not want to imply this PR changed that part.
And agreed on hudi-common and parquet-avro being correctly kept.
There was a problem hiding this comment.
One more thing I found on a re-check, worth your call before this merges.
Nine other bundles already publish a reduced POM and none of them sets promoteTransitiveDependencies: hudi-spark-bundle, hudi-utilities-bundle, hudi-utilities-slim-bundle, hudi-flink-bundle, hudi-cli-bundle, hudi-kafka-connect-bundle, hudi-integ-test-bundle, hudi-metaserver-server-bundle (shade's default) and hudi-timeline-server-bundle (explicit true).
hudi-spark3.5-bundle_2.12's published POM has carried 8 dependencies from 0.15.0 through current master, so dropping non-bundled transitives is the shipped behaviour for those nine. With promotion the seven here publish 73-191 instead. So the real choice is promotion everywhere or promotion nowhere, and right now this PR makes seven differ from nine.
I have kept promotion because it preserves the runtime contract you raised and is the conservative direction for consumers, but I did not want to decide house style silently. Either is a small change from here: drop it and document the transitive drop instead, or keep it and align the other nine in a follow-up. I left those nine alone deliberately, since touching them changes published metadata for nine more artifacts including the Spark and Flink bundles.
There was a problem hiding this comment.
Decision: keep promoteTransitiveDependencies=true on these seven. Not blocking on aligning the other nine.
The three options are not symmetric:
| POM state | what a consumer resolves |
|---|---|
today (createDependencyReducedPom=false) |
full closure, including unrelocated copies of what the jar already absorbed |
| reduced, promotion off | direct deps minus absorbed -> loses real runtime deps |
| reduced, promotion on (this PR) | full closure minus absorbed |
Only the third preserves the runtime contract, so this is not really "house style vs correctness". The nine get away with promotion-off because the Spark/Flink bundles land on a classpath that already supplies everything; these seven land on Hive/Presto/MR classpaths that do not ship orc-core, rocksdbjni, jetty-server or dropwizard metrics. Leaving the nine alone here is the right scope.
Verified on a clean build of ed3ccf5 vs 633d142:
- all seven emit a reduced POM (74 / 80 / 82 / 105 / 112 / 190 / 74 deps)
- cross-checked every entry against each bundle's effective artifactSet (own includes plus the inherited root includes): zero overlap, nothing that is inside a jar is still declared
- every promoted entry carries an explicit
<version>,<parent>is retained, no test/optional/classifier artifacts leaked in hudi-presto-bundle,hudi-hadoop-mr-bundleandhudi-datahub-sync-bundlejar listings are identical to master, and presto keeps its 109org/apache/hudi/hadoop/**entries
Action for this PR (description only): add a line to Impact noting that promotion flattens the graph, so those 80-190 artifacts become depth-1 for consumers with pinned versions and baked-in exclusions, and therefore win mediation against the consumer's own deeper transitives. That differs from both today's behaviour and the other nine.
Follow-up, not here: align the nine, or record an explicit decision that they stay as they are.
| </relocation> | ||
| </relocations> | ||
| <createDependencyReducedPom>false</createDependencyReducedPom> | ||
| <createDependencyReducedPom>true</createDependencyReducedPom> |
There was a problem hiding this comment.
Least surprising of the seven: parquet-avro and avro are already provided so they survive the reduction, and the three removed Hudi deps are all in the artifactSet. The transitive-drop caveat from the aws-bundle comment still applies to whatever hudi-common was supplying that is not bundled here.
There was a problem hiding this comment.
Thanks — the transitive-drop point is addressed for this bundle too: promoteTransitiveDependencies=true is now set on all seven, so hudi-datahub-sync-bundle's reduced POM keeps the dependencies that are not absorbed into the shaded jar rather than dropping them with the removed entry. Details and the numbers are in my reply on the hudi-aws-bundle thread, including a correction about which hadoop/hive artifacts were actually in the closure.
There was a problem hiding this comment.
Two corrections after rebuilding ed3ccf5, neither blocking.
1. parquet-avro and avro no longer survive the reduction. That held for 73d9d06, but not after promotion was added. Shade runs at requiresDependencyResolution=runtime, so project.getArtifacts() is compile+runtime only, and with promoteTransitiveDependencies=true that set replaces the original direct-dependency list rather than filtering it, so provided entries are dropped. On the built POM:
packaging/hudi-datahub-sync-bundle/target/dependency-reduced-pom.xml
parquet-avro : absent
avro : absent
Consumer impact is nil, since provided is not transitive, and this is the only one of the seven that declares any provided deps. So it is a description fix, not a code fix.
2. The reduced POM declares hudi-hadoop-mr:compile, and that artifact depends on hudi-common and hudi-hadoop-common, both of which are in this bundle's artifactSet. The absorbed artifacts come back one hop down. Not a regression, since today's POM declares hudi-common directly anyway, just an incomplete win worth knowing about.
Action: description only. Drop the "already provided so they survive" reasoning and note the provided drop instead. No POM change needed.
…r POM reduction Addresses review feedback on the dependency-reduced POM change. hudi-presto-bundle shades org.apache.hudi:hudi-hadoop-common and org.apache.hudi:hudi-hadoop-mr but declared neither, reaching both only through hudi-hadoop-mr-bundle. Reducing that bundle's POM to zero dependencies made both includes match nothing, and shade does not fail on an unmatched include. Verified by installing the reduced POM and building the presto bundle without -am: the jar went from 109 org/apache/hudi/hadoop/** entries to 0, losing HoodieParquetInputFormat. Declaring both directly restores 109 and stops this bundle's shade inputs depending on another bundle's published dependency list. promoteTransitiveDependencies defaults to false, so reduction also removed artifacts that arrived only through a removed dependency and are not in the shaded jar. Setting it to true on all seven bundles keeps them: the hudi-hadoop-mr-bundle reduced POM goes from 0 to 79 dependencies, restoring orc-core, rocksdbjni, jetty, dropwizard metrics, simpleclient, lz4-java, disruptor and jaxb-api among others. All seven bundles build and emit a reduced POM.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR flips createDependencyReducedPom to true and adds promoteTransitiveDependencies=true on seven shaded bundles so the published POMs no longer declare artifacts already absorbed into the jars, plus declares hudi-hadoop-mr/hudi-hadoop-common directly on the presto bundle. The substantive edges — artifactSet membership for the reduced POM and the transitive-drop behavior of promoteTransitiveDependencies — have already been worked through in the prior round's inline discussion and addressed in ed3ccf5. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
|
Re-reviewed The presto fix in particular is structural, not incidental: declaring Remaining action items, none of which need a new commit except the description edits: This PR:
Follow-ups, please file rather than fix here:
Open question on promotion is answered in the Two things worth crediting: your correction that the |
5 participants
Describe the issue this Pull Request addresses
Closes #16407 (HUDI-7455).
Seven bundles set
<createDependencyReducedPom>false</createDependencyReducedPom>, so the POM publishedto Maven Central still declares the artifacts that were shaded into the jar. Consumers inherit those
artifacts as transitive dependencies even though the classes are already inside the bundle.
What suggests this was unintended rather than deliberate: each of those bundles also sets
<dependencyReducedPomLocation>${project.build.directory}/dependency-reduced-pom.xml</dependencyReducedPomLocation>in the same
<configuration>block,dependency-reduced-pom.xmlis already in.gitignore, andhudi-timeline-server-bundlealready sets the flag totrue— as do the nine bundles that don't overrideshade's default.
Summary and Changelog
createDependencyReducedPom=trueon the seven bundles that had it disabled:hudi-aws-bundle,hudi-hadoop-mr-bundle,hudi-datahub-sync-bundle,hudi-presto-bundle,hudi-azure-bundle,hudi-hive-sync-bundle,hudi-gcp-bundle.promoteTransitiveDependencies=trueon the same seven. The flag defaults to false, which makes shade dropthe removed dependency and everything reaching the project only through it — including artifacts that are
not in the shaded jar. With it enabled the reduced POM keeps the runtime contract minus what was absorbed.
hudi-hadoop-mr-bundle's reduced POM goes from 0 dependencies to 79, restoringorc-core,rocksdbjni,jetty-server/jetty-servlet, dropwizard metrics,simpleclient,tally-core,lz4-java,disruptorand
jaxb-apiamong others. The other six land between 73 and 191.hudi-presto-bundlenow declareshudi-hadoop-mrandhudi-hadoop-commondirectly. It shades both, butreached them only transitively through
hudi-hadoop-mr-bundle. Once that bundle's POM was reduced, the twoincludes matched nothing and shade silently wrote a smaller jar: 109
org/apache/hudi/hadoop/**entriesdropped to 0, losing
HoodieParquetInputFormat, which is the reason the bundle exists. Declaring themdirectly restores 109 and stops this bundle's shade inputs depending on another bundle's published
dependency list.
Verification
The only meaningful check here is install-then-resolve-from-repository, because an in-reactor build never
sees the reduced POM: shade mutates
project.getOriginalModel()and callssetFile(reducedPom), whileMaven's
ReactorReaderserves the effective model, which shade does not touch.mvn package -pl packaging/<bundle>with a stale un-reduced POM in~/.m2also passes, which invalidated my first attempt atverifying this.
org/apache/hudi/hadoop/**entriesAll seven bundles build and emit a reduced POM.
apache-rat:checkpasses.Open question: consistency with the nine already-reduced bundles
Worth deciding before this merges. Nine other bundles already publish a reduced POM —
hudi-spark-bundle,hudi-utilities-bundle,hudi-utilities-slim-bundle,hudi-flink-bundle,hudi-cli-bundle,hudi-kafka-connect-bundle,hudi-integ-test-bundle,hudi-metaserver-server-bundle(shade's default) andhudi-timeline-server-bundle(explicitlytrue) — and none of them setspromoteTransitiveDependencies.hudi-spark3.5-bundle's published POM has carried 8 dependencies from0.15.0 through to current master, so dropping non-bundled transitives is the shipped behaviour of those nine.
With promotion enabled the seven bundles here publish 73–191 dependencies instead, which is safer for
consumers but different from house style. So the choice is really: promotion everywhere, or promotion nowhere.
I have kept promotion on these seven because it preserves the runtime contract, which was the concern raised
in review, and because it is the conservative direction for a consumer. Happy to go either way:
promoteTransitiveDependencieshere, matching the nine, and document the transitive drop instead; orDeliberately not expanding to those nine in this PR — that would change published metadata for nine more
artifacts, including the Spark and Flink bundles, and each needs its own verification.
Impact
This changes the published POM of seven artifacts. A downstream build that currently resolves something
transitively through an artifact that is now absorbed into the bundle will need to declare it directly. That
is the intended correction — a shaded bundle should not advertise dependencies it has already absorbed — but
it is visible to consumers and worth a release note. Happy to stage it per bundle if smaller steps are
preferred.
With
promoteTransitiveDependencies=true, dependencies that are not absorbed are still declared, so thechange is limited to the absorbed set rather than to everything that arrived through it.
Note on what is unchanged: Hadoop and Hive artifacts such as
hadoop-common,hadoop-hdfs,hive-jdbcandhive-metastoreareprovidedin the root pom'sdependencyManagement, so they are not transitive and werenever advertised by these bundles. Consumers were already expected to supply them, before and after this PR.
No change to jar contents beyond the presto fix restoring what it used to contain, and no Hudi API, config or
table format change.
Risk Level
medium — it alters published dependency metadata for seven released artifacts. Verified that all seven build,
that each emits a reduced POM, which dependencies are removed versus retained, and that the presto bundle's
contents are byte-for-byte restored under the install-then-resolve recipe that exposed the regression.
Documentation Update
none in the docs tree, but worth a release-note line for the affected bundles, since consumers may need to
declare dependencies they previously received transitively.
Contributor's checklist