Jackie-Jiang commented on code in PR #19448:
URL: https://github.com/apache/pinot/pull/19448#discussion_r4531957241
##########
pom.xml:
##########
@@ -1749,6 +1867,49 @@
<version>${wire.version}</version>
</dependency>
+ <!-- Patched third-party jars, see pinot-dependency-patches/pom.xml -->
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-patched-third-party-jackson-core</artifactId>
+ <version>${project.version}</version>
Review Comment:
**[P1] Manage the patched artifact versions in pinot-bom**
These six `${project.version}` entries resolve in the context of an
inheriting downstream project. For example, a project inheriting Pinot
`1.7.0-SNAPSHOT` but declaring its own version as `2.0.0-SNAPSHOT` manages all
six `pinot-patched-*` dependencies to nonexistent `2.0.0-SNAPSHOT` artifacts,
even when it explicitly imports `pinot-bom:1.7.0-SNAPSHOT`. This overrides the
transitive versions from plugins such as `pinot-s3` and `pinot-parquet`,
breaking dependency resolution in both default and thin modes.
I confirmed this with Maven's model builder: all six patches resolve to the
child's version while the BOM-managed `pinot-s3` remains at the Pinot version.
Please move these entries into `pinot-bom/pom.xml` and remove the root copies,
following the existing guidance above the BOM import.
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-spark-3/pom.xml:
##########
@@ -32,7 +32,14 @@
<url>https://pinot.apache.org/</url>
<properties>
<pinot.root>${basedir}/../../..</pinot.root>
+ <!-- This jar runs inside a JVM Pinot does not control, so it relocates
its libraries in every
+ build mode, including thin-plugins. See shade.jackson.prefix in the
root pom. -->
+
<shade.jackson.prefix>${shade.prefix}.com.fasterxml.jackson</shade.jackson.prefix>
+ <shade.guava.prefix>${shade.prefix}.com.google.common</shade.guava.prefix>
+ <shade.scala.prefix>${shade.prefix}.org.scala-lang</shade.scala.prefix>
Review Comment:
**[P1] Keep the Spark ingestion artifact compatible with thin core**
With `thin-plugins`, running `LaunchSparkDataIngestionJob -pluginsToLoad
pinot-json` puts the relocated Spark fat jar before the unrelocated `pinot-all`
jar: the selected dependency pass excludes `lib` but always includes the Spark
ingestion plugin, and `addAppResource()` appends `pinot-all` afterward.
The fat jar embeds `SegmentGenerationTaskRunner` from
`pinot-batch-ingestion-common`, while Pinot SPI/core are provided. When
`recordReaderSpec.configClassName` is set, that runner calls
`JsonUtils.objectToJsonNode()`. The relocation changes the invocation's return
descriptor to
`org.apache.pinot.shaded.com.fasterxml.jackson.databind.JsonNode`, but the thin
distribution's `JsonUtils` exposes `com.fasterxml.jackson.databind.JsonNode`.
This produces a linkage error on the executor for configured record readers.
The Spark ingestion artifact and its supplied Pinot SPI/core need a
consistent relocation scheme, such as a coherent external ingestion bundle.
Retaining a fat jar while relocating only this side of the boundary is
insufficient. The new filename-selection tests use empty jars, so they cannot
detect this mismatch.
--
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]
