voonhous commented on code in PR #19490:
URL: https://github.com/apache/hudi/pull/19490#discussion_r3711751401
##########
pom.xml:
##########
@@ -980,6 +982,22 @@
<version>${caffeine.version}</version>
</dependency>
+ <!-- Jackson 1.x (asl). Provided by the hadoop/hive runtime, so provided
scope here; declared so a
+ bundle that shades org.codehaus.jackson can depend on the library
directly rather than reaching it
+ through another bundle's published POM. -->
+ <dependency>
+ <groupId>org.codehaus.jackson</groupId>
+ <artifactId>jackson-core-asl</artifactId>
+ <version>${jackson.asl.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.codehaus.jackson</groupId>
+ <artifactId>jackson-mapper-asl</artifactId>
+ <version>${jackson.asl.version}</version>
+ <scope>provided</scope>
+ </dependency>
Review Comment:
**Blocker.** The managed `<scope>provided</scope>` is not inert. Maven
applies a managed *scope* to transitive nodes, not only to modules that declare
the artifact, so this re-scopes jackson-asl across the whole reactor.
A/B of base `637996c5ab20` against this branch:
```
mvn -o -B dependency:tree -Dincludes=org.codehaus.jackson -fae
```
compile/runtime becomes provided in 7 modules that never declare it:
`hudi-hadoop-mr-bundle`, `hudi-cli-bundle_2.12`, `hudi-flink`,
`hudi-examples-flink`, `hudi-examples-k8s`, `hudi-adb-sync`,
`hudi-tests-common`.
Two of those have real consequences:
- `hudi-hadoop-mr-bundle` sets `createDependencyReducedPom` and
`promoteTransitiveDependencies` true
(`packaging/hudi-hadoop-mr-bundle/pom.xml:139,142`), and shade's goal is
`requiresDependencyResolution=runtime`, so provided artifacts are invisible to
it. Both jackson artifacts silently drop out of that bundle's published POM.
That partly undoes what #19433 wrote two lines above at
`packaging/hudi-hadoop-mr-bundle/pom.xml:140` ("Keep dependencies that are not
absorbed into the shaded jar, so the reduced POM still declares what consumers
need at runtime"), for a bundle this PR never mentions. `hudi-hive-sync-bundle`
and `hudi-gcp-bundle` consume that POM.
- `hudi-examples-k8s` shades `<include>*:*</include>`
(`hudi-examples/hudi-examples-k8s/pom.xml:57`). The flip splits the set:
`jackson-core-asl` and `jackson-mapper-asl` go provided while `jackson-jaxrs`
and `jackson-xc` stay runtime, so its fat jar keeps the JAX-RS provider without
the `org.codehaus.jackson.map` classes it calls into.
Managing the version only fixes this. I ran that variant against base: the
reactor-wide diff is then exactly the two `hudi-presto-bundle` lines and
nothing else, zero collateral flips.
```suggestion
<dependency>
<groupId>org.codehaus.jackson</groupId>
<artifactId>jackson-core-asl</artifactId>
<version>${jackson.asl.version}</version>
</dependency>
<dependency>
<groupId>org.codehaus.jackson</groupId>
<artifactId>jackson-mapper-asl</artifactId>
<version>${jackson.asl.version}</version>
</dependency>
```
##########
packaging/hudi-presto-bundle/pom.xml:
##########
@@ -210,6 +205,20 @@
<version>${project.version}</version>
</dependency>
+ <!-- Jackson 1.x. Shaded via the org.codehaus.jackson:* include below and
previously reached only
+ through hudi-hadoop-mr-bundle's published POM; declared directly so
this bundle's shade inputs come
+ from libraries rather than from another bundle. compile scope because
the classes are shaded in. -->
+ <dependency>
+ <groupId>org.codehaus.jackson</groupId>
+ <artifactId>jackson-core-asl</artifactId>
+ <scope>compile</scope>
+ </dependency>
Review Comment:
**Blocker.** "No change to the presto bundle's jar contents" holds only for
the install-then-resolve recipe. On the path that builds the released jar it
goes 0 -> 623.
A reactor build resolves `hudi-hadoop-mr-bundle` from the reactor and never
sees its reduced POM, so on master this bundle pulls no jackson-asl at all:
```
mvn -o -B dependency:tree -Dincludes=org.codehaus.jackson -fae
```
`hudi-presto-bundle` is absent from the jackson list on base `637996c5ab20`
and appears only on this branch, at compile scope. The `org.codehaus.jackson:*`
include at line 81 then matches, and the 623 classes land in the jar.
That is the path that ships. `scripts/release/deploy_staging_jars.sh:71`
deploys this bundle with `-Dscala-2.12 -Dspark3.5 -Dflink1.20
-Ddocker.compose.skip=true`, full reactor, no `-pl`. The released jars agree:
```
for v in 0.15.1 1.0.2 1.1.0-SNAPSHOT 1.2.0-SNAPSHOT; do
unzip -l hudi-presto-bundle-$v.jar | grep -c org/codehaus/jackson/
done
# 651, 0, 0, 0
```
0.15.1 shipped Jackson 1.x; every release since ships none, and a
1.3.0-SNAPSHOT reactor build from before #19433 also has 0. So the 7313/623
master baseline is a one-day-old side effect of #19433 (merged 2026-08-03), not
this bundle's shipped contract, and this PR would make it permanent on both
build paths.
Please re-measure with a full-reactor `mvn install -DskipTests` and put that
number in the PR body beside the repository-resolution number. If it reads 0 ->
623, take the smaller fix described in my next comment rather than declaring
these two artifacts. Worth noting too that #19491 asserts nothing about
`org/codehaus/jackson/**` and only exercises the repository-resolution path, so
it would not catch this either.
##########
pom.xml:
##########
@@ -980,6 +982,22 @@
<version>${caffeine.version}</version>
</dependency>
+ <!-- Jackson 1.x (asl). Provided by the hadoop/hive runtime, so provided
scope here; declared so a
+ bundle that shades org.codehaus.jackson can depend on the library
directly rather than reaching it
+ through another bundle's published POM. -->
Review Comment:
**Minor, the stated rationale is not the actual path.** "Provided by the
hadoop/hive runtime" is what justifies the `provided` scope, but hadoop and
hive are managed `provided` in this pom and do not propagate. The compile-scope
path that actually exists in the reactor is parquet:
```
org.apache.parquet:parquet-avro:1.10.1:compile <-
hudi-hadoop-mr-bundle, at ${hive.parquet.version} (pom.xml:131)
\- org.apache.parquet:parquet-hadoop:1.10.1:compile
+- org.codehaus.jackson:jackson-mapper-asl:1.9.13:compile
\- org.codehaus.jackson:jackson-core-asl:1.9.13:compile
```
`parquet-hadoop` 1.10.1 declares both artifacts; 1.15.2, which the presto
bundle uses, declares neither. The other path is `hadoop-common:2.10.2`, and
that one is profile-dependent: hadoop-common 3.3.6 and 3.4.0 declare no
`org.codehaus.jackson` at all, so under the hadoop3 profiles nothing in the
runtime provides it and `provided` is simply wrong.
If the root entries survive review, restate this as "reached at compile
scope via parquet-hadoop 1.10.1 and hadoop-common 2.10.2 (hadoop2 profile
only)". The same correction applies to the property comment at line 132. The
durable fix for this whole class of problem is moving `hive.parquet.version`
off 1.10.1, which is what drags Jackson 1.x in at all.
##########
packaging/hudi-presto-bundle/pom.xml:
##########
@@ -191,11 +191,6 @@
<artifactId>hudi-common</artifactId>
<version>${project.version}</version>
</dependency>
- <dependency>
- <groupId>org.apache.hudi</groupId>
- <artifactId>hudi-hadoop-mr-bundle</artifactId>
- <version>${project.version}</version>
- </dependency>
Review Comment:
**Major, verification gap.** The jar listing is only half the contract.
#19433 was about the published dependency-reduced POM, and removing this
dependency changes it: with `promoteTransitiveDependencies=true`, everything
that reached this bundle only through `hudi-hadoop-mr-bundle` leaves the presto
bundle's published POM. Diffing the two installed jars'
`META-INF/DEPENDENCIES`, that includes `it.unimi.dsi:fastutil:7.0.13` and
`org.apache.parquet:parquet-format:2.4.0`, and it returns the parquet
transitives from 1.10.1 to `${presto.parquet.version}`.
That is probably a good change, but the PR neither claims nor measures it.
Please build master and this branch and paste the `diff` of
`packaging/hudi-presto-bundle/target/dependency-reduced-pom.xml` into the
verification section, so the metadata half is covered the way the jar half is.
The same matrix should cover the two bundles that still depend on
`hudi-hadoop-mr-bundle` and are affected by the root pom change in this PR:
`packaging/hudi-hive-sync-bundle/pom.xml:167` and
`packaging/hudi-gcp-bundle/pom.xml:157`. #19469's question 3 is currently
answered for one of the three bundles. Either extend the same pattern to those
two here, or state in the PR body that they are deliberately left for a
follow-up and link it.
##########
packaging/hudi-presto-bundle/pom.xml:
##########
@@ -210,6 +205,20 @@
<version>${project.version}</version>
</dependency>
+ <!-- Jackson 1.x. Shaded via the org.codehaus.jackson:* include below and
previously reached only
Review Comment:
Nit, feel free to ignore: the include is at line 81, above this block, not
below. The sibling comment at line 194 gets the direction right ("artifactSet
above").
```suggestion
<!-- Jackson 1.x. Shaded via the org.codehaus.jackson:* include above
and previously reached only
```
##########
pom.xml:
##########
@@ -129,6 +129,8 @@
<hive.groupid>org.apache.hive</hive.groupid>
<hive.version>2.3.10</hive.version>
<hive.parquet.version>1.10.1</hive.parquet.version>
+ <!-- Jackson 1.x, reached through the hadoop/hive stack; shaded by
hudi-presto-bundle -->
+ <jackson.asl.version>1.9.13</jackson.asl.version>
Review Comment:
Nit, feel free to ignore: this splits the hive property block, landing
between `hive.parquet.version` (line 131) and `hive.avro.version` (line 134).
Moving it below `hive.avro.version`, or next to the `fasterxml.jackson.*`
properties at lines 103-106, keeps the grouping intact.
##########
packaging/hudi-presto-bundle/pom.xml:
##########
@@ -210,6 +205,20 @@
<version>${project.version}</version>
</dependency>
+ <!-- Jackson 1.x. Shaded via the org.codehaus.jackson:* include below and
previously reached only
+ through hudi-hadoop-mr-bundle's published POM; declared directly so
this bundle's shade inputs come
+ from libraries rather than from another bundle. compile scope because
the classes are shaded in. -->
+ <dependency>
+ <groupId>org.codehaus.jackson</groupId>
+ <artifactId>jackson-core-asl</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.codehaus.jackson</groupId>
+ <artifactId>jackson-mapper-asl</artifactId>
+ <scope>compile</scope>
+ </dependency>
Review Comment:
**Major.** This re-litigates HUDI-4997 without citing it, and the history
points at a smaller fix.
Commit `779a96506fb7` (#6893, 2022-10-20, "Use jackson-v2 import instead of
jackson-v1") deleted exactly this pairing: the `codehaus-jackson.version`
property and the `jackson-core-asl` / `jackson-mapper-asl` / `jackson-jaxrs` /
`jackson-xc` block from the root pom, the `org.codehaus.jackson.` relocation
from this file, and the two jackson-asl lines from
`dependencies/hudi-presto-bundle.txt`. Stated reason in that PR: jackson-v1 has
security risks.
```
gh api repos/apache/hudi/commits/779a96506fb7 --jq
'.files[]|select(.filename=="pom.xml")|.patch'
git show 779a96506fb7 -- packaging/hudi-presto-bundle/pom.xml
```
The `org.codehaus.jackson:*` include at line 81 was added together with that
relocation in `4e050cc2ba26` (#2816, 2021-04-17), back when Hudi's own code
imported jackson-v1. #6893 removed the relocation and left the include
orphaned. Nothing in the tree references the package today:
```
grep -rn "org\.codehaus\.jackson" --include="*.java" --include="*.scala" . |
grep -v /target/ # no hits
```
So these are dead classes, and they ship unrelocated at their original
coordinates onto Presto's classpath. `jackson-mapper-asl` 1.9.13 is also the
top of CVE-2019-10172's affected range, with no fixed release in the ASL 1.x
line.
Concrete alternative: rather than adding these two dependencies, delete the
dead `<include>org.codehaus.jackson:*</include>` at line 81 in this PR. Both
build paths then agree at 0 `org/codehaus/jackson/` entries, matching 1.0.2
through 1.2.0 and pre-#19433 master, the root pom change becomes unnecessary,
and it finishes the cleanup #6893 started. If you would rather keep shipping
Jackson 1.x, cite #6893 in the PR body with the reason it is acceptable to
re-add it, and add a relocation consistent with the avro and commons-lang3 ones
already in this file.
##########
packaging/hudi-presto-bundle/pom.xml:
##########
@@ -210,6 +205,20 @@
<version>${project.version}</version>
</dependency>
+ <!-- Jackson 1.x. Shaded via the org.codehaus.jackson:* include below and
previously reached only
+ through hudi-hadoop-mr-bundle's published POM; declared directly so
this bundle's shade inputs come
+ from libraries rather than from another bundle. compile scope because
the classes are shaded in. -->
Review Comment:
Minor: `dependencies/hudi-presto-bundle.txt` is the tracked dependency
manifest for this bundle, and #6893 edited it in the same commit that removed
these two artifacts (`git show 779a96506fb7 --
dependencies/hudi-presto-bundle.txt`). Making them direct compile dependencies
leaves that file wrong again.
What keeps this minor: it is already stale (`grep -c hbase
dependencies/hudi-presto-bundle.txt` returns 10 although HBase was removed in
#12964) and `scripts/dependency.sh` is referenced by no workflow. So either
regenerate it with `scripts/dependency.sh`, or add a line to the PR body noting
that `dependencies/` is unmaintained. Just do not leave it silently more wrong
than it already is.
--
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]
