voonhous commented on code in PR #19433:
URL: https://github.com/apache/hudi/pull/19433#discussion_r3694767642
##########
packaging/hudi-datahub-sync-bundle/pom.xml:
##########
@@ -91,7 +91,7 @@
<shadedPattern>org.apache.hudi.org.openjdk.jol.</shadedPattern>
</relocation>
</relocations>
- <createDependencyReducedPom>false</createDependencyReducedPom>
+ <createDependencyReducedPom>true</createDependencyReducedPom>
Review 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.
**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: none.** My earlier "**Action:** description only, drop the
already-provided reasoning" was wrong -- I went back and checked the PR
description, and that reasoning only ever appeared in this thread, never in the
description. So there is nothing to fix there. No POM change either. Recording
the two points above for the archive and resolving.
##########
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.
--
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]
