rangareddy commented on code in PR #19490:
URL: https://github.com/apache/hudi/pull/19490#discussion_r3712378497
##########
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:
Confirmed and dropped — the root pom is untouched in the new revision, so
this collateral is gone entirely.
I reproduced your A/B on `637996c5ab20` vs the old branch tip and got your
list exactly. Parsing the per-module trees into sets, 8 modules changed, 7 of
which never declare the artifact:
```
hudi-adb-sync compile -> provided
hudi-cli-bundle_2.12 compile -> provided
hudi-examples-flink compile -> provided
hudi-examples-k8s runtime -> provided
hudi-flink compile -> provided
hudi-tests-common compile -> provided
hudi-hadoop-mr-bundle compile -> provided
hudi-presto-bundle (absent) -> compile
```
And the `hudi-examples-k8s` split is exactly as you described —
`jackson-core-asl` and `jackson-mapper-asl` go `provided` while
`jackson-jaxrs:1.9.13` and `jackson-xc:1.9.13` stay `runtime`, so its `*:*` fat
jar would have kept the JAX-RS provider without `org.codehaus.jackson.map`. I
had reasoned "managed scope is inert for modules that do not declare the
artifact", which is simply not how Maven applies a managed scope to transitive
nodes. Thanks for the correction.
I did not take the manage-version-only suggestion either, because your next
comment made the dependencies unnecessary in the first place — see there.
##########
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:
You are right, and this was the important one. I re-measured with `mvn
install -DskipTests -Dscala-2.12 -Dspark3.5 -Dflink1.20` on both paths — `-pl
packaging/hudi-presto-bundle` alone for repository resolution, and `-pl
packaging/hudi-hadoop-mr-bundle,packaging/hudi-presto-bundle` so the mr-bundle
comes from the reactor:
```
variant repository path reactor path (ships)
master 637996c5ab20 7273 cls / 651 jackson 6650 cls / 0
old revision of this PR 7273 / 651 7273 / 651 <-- 0 ->
651
new revision 6650 / 0 6650 / 0
```
So it read `0 -> 623` classes (651 zip entries) on the path that ships,
exactly as you predicted, and my "7313 before and after" was a
repository-path-only measurement. Both numbers are now in the PR body with the
recipe, and I added an explicit retraction paragraph at the top so anyone
arriving mid-thread is not misled by the original claim.
Also confirms your point about #19491: it asserts required classes and a
`org/apache/hudi/hadoop/**` floor, and nothing about `org/codehaus/jackson/**`,
so it would not have caught this. The new revision leaves that jar at 109 such
entries, comfortably above the floor of 100.
##########
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:
Taken — this is now what the PR does. Every claim here checks out:
- `779a96506fb7` (#6893) removed the `codehaus-jackson.version` property and
all four asl artifacts from the root pom, the `org.codehaus.jackson.`
relocation from this file, and the two asl lines from
`dependencies/hudi-presto-bundle.txt`, in one commit.
- `grep -rn "org\.codehaus\.jackson" --include="*.java" --include="*.scala"
. | grep -v /target/` returns nothing, so the include at line 81 has matched
only transitively-reached classes since then, and they ship unrelocated because
#6893 took the relocation with it.
So the PR is now two deletions in this file and nothing else: the
`hudi-hadoop-mr-bundle` dependency, and the dead include. Both paths land at 0
`org/codehaus/jackson/` entries and 6650 classes, matching 1.0.2 through 1.2.0
and master reactor builds. The root pom change is gone, which also disposes of
your first comment and the two nits below it.
I chose deletion over re-adding with a relocation for the reason you give:
`jackson-mapper-asl` 1.9.13 sits at the top of CVE-2019-10172 with no fixed ASL
1.x release, and nothing in the tree calls into it. #6893 is cited in both the
commit message and the PR body.
##########
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:
Measured and in the PR body. Parsing both reduced POMs into
`groupId:artifactId:version:scope` sets rather than diffing the XML (the raw
diff is noisy with reordering), master vs this branch:
**Reactor path** — 81 -> 80, exactly one entry, which is the whole point of
the PR:
```
- org.apache.hudi:hudi-hadoop-mr-bundle:1.3.0-SNAPSHOT:compile
```
**Repository path** — 81 -> 80, and your prediction is precisely right:
```
- it.unimi.dsi:fastutil:7.0.13:compile
- org.apache.parquet:parquet-format:2.4.0:compile
- commons-lang:commons-lang:2.6, org.apache.orc:orc-shims:1.6.0
- org.apache.parquet:{column,common,encoding,hadoop,jackson}:1.10.1
+ org.apache.parquet:{column,common,encoding,hadoop}:1.15.2,
parquet-format-structures:1.15.2, parquet-jackson:1.15.2:runtime
+ org.apache.orc:orc-shims:1.9.1, com.github.luben:zstd-jni:1.5.6-6,
javax.annotation:javax.annotation-api:1.3.2
```
`fastutil` and `parquet-format:2.4.0` leave, and the parquet transitives
return from 1.10.1 to `${presto.parquet.version}` — the version this bundle
actually shades.
On `hudi-hive-sync-bundle` and `hudi-gcp-bundle`: since the root pom is no
longer touched, nothing in this PR reaches them. I have stated in the PR body
that they are deliberately left for a follow-up rather than swept in, because
each needs its own before/after jar plus reduced-POM measurement on both paths
— which is precisely the work this review round showed cannot be skipped. I
will open the follow-up referencing #19469 question 3 and link it here.
##########
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:
Correct, and moot now — the root pom entries are gone, so there is no
rationale left to restate. Recording the finding anyway since it outlives this
PR: the compile-scope path is `hudi-hadoop-mr-bundle ->
parquet-avro:${hive.parquet.version}=1.10.1 -> parquet-hadoop:1.10.1 ->
jackson-{core,mapper}-asl:1.9.13`, and `parquet-hadoop:1.15.2` declares
neither, which is why the presto bundle at `${presto.parquet.version}` has no
path of its own. My "provided by the hadoop/hive runtime" was wrong on both
counts — hadoop and hive are managed `provided` here and do not propagate, and
hadoop-common 3.3.6/3.4.0 declare no `org.codehaus.jackson` at all, so
`provided` would have been false under the hadoop3 profiles.
Agreed that moving `hive.parquet.version` off 1.10.1 is the durable fix for
the class of problem. That is well outside this PR; worth its own issue.
##########
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:
Right, and the block that comment belonged to is deleted in the new
revision, so it is gone rather than corrected. Good catch either way — I had
the direction backwards while the sibling comment at line 194 has it right.
##########
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:
Moot — the root pom is untouched in the new revision, so the property is not
added and the hive block stays intact.
--
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]
