voonhous commented on code in PR #19433:
URL: https://github.com/apache/hudi/pull/19433#discussion_r3701742792
##########
packaging/hudi-presto-bundle/pom.xml:
##########
@@ -141,7 +141,7 @@
<shadedPattern>org.apache.hudi.org.openjdk.jol.</shadedPattern>
</relocation>
</relocations>
- <createDependencyReducedPom>false</createDependencyReducedPom>
+ <createDependencyReducedPom>true</createDependencyReducedPom>
Review Comment:
Picked up separately as agreed: filed as #19469, low priority, assigned to
you.
I kept your framing rather than mine -- the issue does not assert the
dependency should go, it asks what `hudi-hadoop-mr-bundle` is currently
supplying to the shade inputs now that `hudi-hadoop-mr` and
`hudi-hadoop-common` are declared directly, and sets the bar at a
byte-identical jar listing before anything is removed.
One caveat recorded there: CI cannot catch a regression on this today, so
the 109-entry check has to be run by hand with the install-then-resolve recipe.
That is #19468.
##########
packaging/hudi-aws-bundle/pom.xml:
##########
@@ -117,7 +117,7 @@
<shadedPattern>org.apache.hudi.aws.org.apache.httpcomponents.</shadedPattern>
</relocation>
</relocations>
-
<createDependencyReducedPom>false</createDependencyReducedPom>
+
<createDependencyReducedPom>true</createDependencyReducedPom>
Review Comment:
Promotion stays on for these seven, as above. Nothing further needed on this
PR, so resolving.
On the "align the other nine" follow-up: I went and looked at the actual
state, and the fix is not "add promotion to nine more bundles". Recording it
here so the issue can be filed accurately.
**18 modules run shade, not 16** -- the 16 packaging bundles plus `hudi-io`
and `hudi-examples-k8s`.
- **`hudi-io` also sets `createDependencyReducedPom=false`, and it is right
to.** It is the only module in the repo with `shadedArtifactAttached=true`: the
shaded jar ships under a `shaded` classifier and the main artifact stays
unshaded, so its POM has to keep declaring `protobuf-java`. Reducing it would
strip a dependency the primary jar genuinely needs. So the invariant is not
"false is wrong", it is **false is correct iff the shaded jar is attached under
a classifier**. All 16 packaging bundles replace the main artifact, so for them
it is wrong. Worth stating explicitly, because otherwise the next person reads
this PR and "fixes" `hudi-io`.
- **The nine already publish reduced POMs**, so their only gap is promotion.
That puts them in the middle row of the table above, the one that loses real
runtime deps.
- **The "lands on a classpath that already supplies everything" argument
only covers four of the nine**: `hudi-spark-bundle`, `hudi-utilities-bundle`,
`hudi-utilities-slim-bundle`, `hudi-flink-bundle`. It does not cover
`hudi-cli-bundle`, `hudi-kafka-connect-bundle`, `hudi-integ-test-bundle`,
`hudi-metaserver-server-bundle` or `hudi-timeline-server-bundle`. That is the
real exposure, and it is five bundles rather than nine -- those should be
verified first.
- **`hudi-timeline-server-bundle` is an outlier twice over**: the only
bundle setting `createDependencyReducedPom` explicitly true, and the only one
not setting `dependencyReducedPomLocation`, so it writes to the module basedir
instead of `target/`. Gitignored and the published POM is identical, so
cosmetic, but it means no two bundles in the repo currently agree on the full
flag triple.
**Suggested shape for the follow-up.** The root pom already carries
`maven-shade-plugin` in `<build><plugins>` with a shared `<configuration>` --
artifactSet plus relocations -- that every bundle inherits and extends via
`combine.children="append"`. `createDependencyReducedPom`,
`promoteTransitiveDependencies` and `dependencyReducedPomLocation` belong there
once, with the per-bundle copies deleted and a single documented override in
`hudi-io` explaining the classifier reason. `hudi-examples-k8s` needs a
separate look since it inherits too.
That fixes the drift rather than adding a tenth copy of it, and it means the
next bundle added to `packaging/` inherits the right behaviour instead of
relying on someone remembering. The per-bundle install-then-resolve
verification is still the real cost, which is why it does not belong in this PR.
**Filed as #19466**, assigned to rangareddy, with the dead
`org.apache.httpcomponents.` relocation entry from the thread above folded into
the same sweep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
