voonhous commented on code in PR #19433:
URL: https://github.com/apache/hudi/pull/19433#discussion_r3689941782
##########
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:
`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.)
##########
packaging/hudi-hive-sync-bundle/pom.xml:
##########
@@ -123,7 +123,7 @@
<shadedPattern>org.apache.hudi.org.objenesis.</shadedPattern>
</relocation>
</relocations>
- <createDependencyReducedPom>false</createDependencyReducedPom>
+ <createDependencyReducedPom>true</createDependencyReducedPom>
Review Comment:
Safe on the edge that breaks presto: this bundle declares `hudi-hive-sync`
directly, and that brings `hudi-hadoop-mr` and `hudi-hadoop-common`, so its
shade inputs do not depend on `hudi-hadoop-mr-bundle`'s POM.
The transitive-drop point from the aws-bundle comment does apply though:
removing `hudi-hive-sync` takes the `hive-*` and `hadoop-*` compile deps with
it. And the one dependency left in the reduced POM is `hudi-hadoop-mr-bundle`,
so this publishes as a bundle whose sole declared dependency is another fat
bundle duplicating its own contents.
##########
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:
Companion to the `hudi-hadoop-mr-bundle` comment.
Lines 69 and 71 shade `hudi-hadoop-common` and `hudi-hadoop-mr`, but the
only path to either is the transitive edge through `hudi-hadoop-mr-bundle` at
line 193. Once that bundle's POM is reduced to nothing, both includes match
nothing and this jar silently drops 109 `org/apache/hudi/hadoop/**` classes
including `HoodieParquetInputFormat`, which is the reason the bundle exists.
Declaring `hudi-hadoop-mr` and `hudi-hadoop-common` directly here fixes it,
and is worth doing on its own merit: a bundle's shade inputs should not depend
on what another bundle happens to leak transitively.
Separately, after reduction this POM's only remaining dependency is
`hudi-hadoop-mr-bundle`, a ~30 MB fat jar duplicating classes this bundle
already shades. Pre-existing, but the reduced POM promotes it to the headline
of the published metadata.
##########
packaging/hudi-gcp-bundle/pom.xml:
##########
@@ -115,7 +115,7 @@
<shadedPattern>org.apache.hudi.org.openjdk.jol.</shadedPattern>
</relocation>
</relocations>
- <createDependencyReducedPom>false</createDependencyReducedPom>
+ <createDependencyReducedPom>true</createDependencyReducedPom>
Review Comment:
Same as hive-sync: safe on the `hudi-hadoop-mr-bundle` edge, because
`hudi-hive-sync` is a direct dependency here and supplies `hudi-hadoop-mr` /
`hudi-hadoop-common`. The transitive-drop point from the aws-bundle comment
still applies.
##########
packaging/hudi-azure-bundle/pom.xml:
##########
@@ -132,7 +132,7 @@
<shadedPattern>org.apache.hudi.org.reactivestreams.</shadedPattern>
</relocation>
</relocations>
- <createDependencyReducedPom>false</createDependencyReducedPom>
+ <createDependencyReducedPom>true</createDependencyReducedPom>
Review Comment:
Transitive-drop point from the aws-bundle comment applies here too:
`hudi-hive-sync` is removed, and the `hive-*` and `hadoop-*` compile deps that
only arrive through it go with it. `hudi-common` correctly stays, since it is
not bundled.
##########
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:
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.
##########
packaging/hudi-aws-bundle/pom.xml:
##########
@@ -117,7 +117,7 @@
<shadedPattern>org.apache.hudi.aws.org.apache.httpcomponents.</shadedPattern>
Review Comment:
Small correction to the description, which says httpclient/httpcore are
relocated to `org.apache.hudi.aws.org.apache.httpcomponents`.
`org.apache.httpcomponents` is the groupId, not a Java package. Those
classes live under `org.apache.http.*`, so this relocation matches nothing.
They *are* relocated, but by the inherited root rule `org.apache.http.` ->
`org.apache.hudi.org.apache.http.` in the root `pom.xml`, which applies only
because `<relocations combine.children="append">` merges the parent config. I
checked the built mr-bundle jar: everything is under
`org/apache/hudi/org/apache/http/`, nothing unrelocated, nothing under
`org/apache/hudi/aws/`.
The conclusion holds and dropping them from the POM is fine, the stated
evidence just is not what actually happened. This relocation entry is dead
config and could go in a follow-up.
##########
packaging/hudi-hadoop-mr-bundle/pom.xml:
##########
@@ -136,7 +136,7 @@
<shadedPattern>org.apache.hudi.com.fasterxml.jackson.</shadedPattern>
</relocation>
</relocations>
- <createDependencyReducedPom>false</createDependencyReducedPom>
+ <createDependencyReducedPom>true</createDependencyReducedPom>
Review Comment:
All five direct deps here (`hudi-common`, `hudi-hadoop-mr`, `kryo-shaded`,
`parquet-avro`, `avro`) are in the artifactSet, so the reduced POM comes out
with zero dependencies, as you noted in the description. Two consequences:
**1. It breaks `hudi-presto-bundle` in non-reactor builds.** Presto shades
`org.apache.hudi:hudi-hadoop-common` and `org.apache.hudi:hudi-hadoop-mr` but
reaches both only transitively through this bundle. `hudi-common` depends on
neither. Once this POM is empty those includes match nothing, and shade does
not fail on an unmatched include, it just writes a smaller jar. The current
presto jar has 109 `org/apache/hudi/hadoop/**` entries,
`HoodieParquetInputFormat` among them.
This will not show up in a single reactor build. Shade 3.5.3 mutates
`project.getOriginalModel()` and calls `project.setFile(reducedPom)`, while
Maven 3.9's `ReactorReader.findModel` returns `project.getModel()`, the
effective model, which shade never touches. So in-reactor consumers still see
the un-reduced list. It appears as soon as this bundle is resolved from a
repository:
```
mvn install -DskipTests # installs the
new, empty POM
mvn package -pl packaging/hudi-presto-bundle -DskipTests # note: no -am
unzip -l packaging/hudi-presto-bundle/target/hudi-presto-bundle-*.jar | grep
-c org/apache/hudi/hadoop/
```
Worth flagging that `mvn package -pl packaging/<bundle>` only reproduces
this *after* the new POM has been installed. With a stale un-reduced POM still
in `~/.m2` it passes, which I suspect is why the verification came back clean.
CI will not catch it either: every `-pl packaging/...` in `.github/workflows`
and `scripts/` passes `-am`, and `packaging/bundle-validation` has no presto
coverage at all.
**2. Non-bundled transitives go too.** This jar carries `hudi-common`
classes that need `orc-core`, `rocksdbjni`, `jetty-server`/`jetty-servlet`,
dropwizard metrics, `simpleclient`, `tally-core`, `lz4-java`, `disruptor` and
`jaxb-api`. I checked the built jar and none of them are in it. After this
change the POM advertises nothing at all, so a Maven consumer gets neither
those classes' host jars nor their dependencies.
--
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]
