xiangfu0 commented on code in PR #19890:
URL: https://github.com/apache/pinot/pull/19890#discussion_r4288284496
##########
pom.xml:
##########
@@ -2977,6 +2977,39 @@
<exclude>shaded/parquet/com/fasterxml/jackson/**</exclude>
<exclude>META-INF/versions/**/shaded/parquet/com/fasterxml/jackson/**</exclude>
+ <!-- Keep Hadoop's shaded JAX-RS providers while using managed
Jackson core and databind. -->
+
<exclude>org/apache/hadoop/shaded/com/fasterxml/jackson/core/**</exclude>
+
<exclude>META-INF/versions/**/org/apache/hadoop/shaded/com/fasterxml/jackson/core/**</exclude>
+
<exclude>org/apache/hadoop/shaded/com/fasterxml/jackson/databind/**</exclude>
+
<exclude>META-INF/versions/**/org/apache/hadoop/shaded/com/fasterxml/jackson/databind/**</exclude>
+
<exclude>org/apache/hadoop/shaded/com/fasterxml/jackson/annotation/**</exclude>
+
<exclude>META-INF/versions/**/org/apache/hadoop/shaded/com/fasterxml/jackson/annotation/**</exclude>
+ <exclude>com/facebook/presto/jdbc/internal/jackson/**</exclude>
Review Comment:
Minor / defensive: this exclude removes *all* of Presto's embedded Jackson,
but the managed Jackson it gets relocated onto only ships core + databind +
annotations. If a Presto-JDBC jar ever flows in bundling an extra module (e.g.
a `jackson.datatype.*`), its references would relocate to a managed package
that has no such class and hit `NoClassDefFoundError`. No impact today since
presto-jdbc isn't a declared dependency here, so this is latent — just worth
narrowing to `.../jackson/core|databind|annotation/**` if Presto becomes a real
transitive input.
##########
pom.xml:
##########
@@ -2999,6 +3032,28 @@
<exclude>META-INF/maven/com.fasterxml.jackson.core/**</exclude>
</excludes>
</filter>
+ <filter>
+ <artifact>org.apache.hadoop*:*</artifact>
+ <excludes>
+ <exclude>META-INF/maven/com.google.guava/**</exclude>
+ <exclude>META-INF/maven/com.fasterxml.jackson.core/**</exclude>
+ <exclude>META-INF/versions/**/module-info.class</exclude>
Review Comment:
Nit: the global `*:*` filter already excludes top-level `module-info.class`
(line 2971) but not the MR-jar versioned ones, which is why these per-artifact
`META-INF/versions/**/module-info.class` excludes exist on Hadoop and Iceberg.
That's fine and targeted; just flagging that if more shaded carriers show up
later, a single global versioned-module-info exclude might be less repetitive
than per-artifact copies.
##########
pinot-plugins/pinot-file-system/pinot-s3/pom.xml:
##########
@@ -35,6 +35,10 @@
</properties>
<dependencies>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-core</artifactId>
+ </dependency>
Review Comment:
Good call adding managed `jackson-core` explicitly here. Since the shade
config strips AWS SDK's embedded
`software.amazon.awssdk.thirdparty.jackson.core` and relocates its references
onto managed Jackson, the managed core has to be present for this isolated
plugin's shaded jar. Consistent with the Pulsar guava addition.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
