gortiz opened a new pull request, #19448:
URL: https://github.com/apache/pinot/pull/19448
## Summary
Pinot shades almost everything it builds, and the shading does not do what
it looks like it does.
The shared shade configuration in the root pom relocates exactly three
packages: `com.fasterxml.jackson`, `com.google.common` and `org.scala-lang`.
Every module relocates them to the same `org.apache.pinot.shaded` prefix, and
the launcher puts `lib/*` on the classpath ahead of the plugin jars, so a
plugin's relocated copy of a class always lost to the distribution's copy of
the same class. Two plugins that relocated the same library collided with each
other for the same reason. Everything else a plugin bundles - kafka-clients,
hadoop, avro, parquet, aws-sdk, netty - was never relocated at all and has
always been merged onto one flat classpath, first match winning.
So the isolation was not real. The costs were:
- **Build time.** Roughly 37 modules opt into shading. The root
`shade.phase.prop` default of `none` looks like it disables this, but almost
every module overrides it to `package` in its own properties or in an
`activeByDefault` profile, so an ordinary `mvn install` shades them all. Each
execution unpacks its full dependency tree, rewrites every class entry and
writes a merged jar. The cost multiplies where the dependency graph converges:
hadoop is embedded independently by hdfs, parquet and orc; aws-sdk by s3 and
kinesis; avro by three modules.
- **Distribution size.** Because every plugin embeds its own copy of every
shared library, the plugin payload is 640 MB for what is really about 330 MB of
distinct content.
- **Legibility.** Version skew between plugins is invisible when it is
buried inside uber-jars. Stack traces name relocated classes, `jar tf` tells
you little, and CVE scanners either flag one library once per fat jar that
embeds it or miss it entirely. `KafkaConfigBackwardCompatibleUtils` exists
solely to rewrite stream configs that name shaded Kafka classes, and its own
comment is a `FIXME` about shade rewriting string constants inside classes.
This PR removes relocation where it was doing nothing and replaces the
per-plugin fat jars with a thin jar plus a shared dependency directory.
## What changed
**Relocation** is removed from the shared shade configuration and
re-declared in the modules where it is genuinely correct - artifacts loaded by
a JVM Pinot does not control:
| Module | Why it still relocates |
|---|---|
| `pinot-java-client`, `pinot-jdbc-client` | Driver jars embedded in a
user's application |
| `pinot-batch-ingestion-spark-3`, `pinot-batch-ingestion-hadoop` | Run
inside Spark / Hadoop executors |
| `pinot-spark-3-connector` | Unchanged; it already had its own shade
configuration and relocations |
**Plugin packaging.** 28 plugin modules stop building a fat jar: the
`shade.phase.prop` property and the now-pointless `pinot-fastdev` /
`build-shaded-jar` profiles are deleted, which is why much of this diff is
deletions. The distribution ships each plugin as its thin module jar under
`plugins/<type>/<name>/`, with plugin dependencies collected into a new shared
top-level `plugin-libs/` directory where each artifact is stored once.
```
lib/pinot-all-<version>.jar core (still shaded, no relocations)
plugin-libs/ shared dependency store, one copy per
artifact
slf4j-api-2.0.18.jar (was embedded in 15 plugin fat jars)
jackson-annotations-2.22.jar (was embedded in 13)
...
plugins/pinot-input-format/pinot-json/
pinot-json-<version>.jar thin jar
pinot-plugin.classpath the store entries this plugin needs
plugins-external/ unchanged: shaded fat jars for Spark /
Hadoop
```
**A per-plugin dependency index.** Collecting dependencies into one
directory removes the information a jar's location used to carry: which plugin
needs it. Two existing consumers depend on that. The launcher's
`PLUGINS_INCLUDE` path selects jars by matching a plugin name against the jar's
parent directory, and `LaunchSparkDataIngestionJobCommand` does the same to
honour `-pluginsToLoad` / `-pluginsToExclude` when deciding what to ship to
Spark executors. With a shared store every dependency's parent directory is
`plugin-libs`, so selection would either ship all of them or none - and
shipping all of them would have sent the Kafka plugin's Scala dependencies to
the executors, which the hard-coded `-pluginsToExclude pinot-kafka-3.0` default
exists to prevent.
Each plugin therefore ships a `pinot-plugin.classpath` next to its jar,
generated by `dependency:build-classpath`, naming the store jars it needs as
distribution-relative paths. The launcher and the Spark command resolve a
selected plugin's dependencies through it; the store is added wholesale only
when nothing was selected. Entries are accepted `:`-separated - the classpath
fragment `build-classpath` actually writes - or newline-separated. A plugin
with no index, such as an old-format plugin that keeps its jars beside its own
jar, contributes nothing extra and keeps working unchanged.
**`pinot-parquet` drops the `hadoop-client-runtime` uber-jar.** That
artifact is Hadoop's own uber-jar of third-party runtime dependencies,
pre-relocated under `org.apache.hadoop.shaded`, and it bundles Jetty for
Hadoop's embedded `HttpServer2` web UIs - which Pinot never starts;
`HttpServer2` is not referenced anywhere in the codebase. That bundled Jetty
carries CVE-2026-2332, and because the classes are baked into the uber-jar
rather than resolved as a dependency node, a Maven exclusion cannot remove
them: the root pom needed a `maven-shade-plugin` filter, which only worked
while the consuming plugin was shaded. Without this change the distribution
would regain 1059 Jetty classes.
`pinot-parquet` was its only consumer, and everything it needs is in
`hadoop-common`, where Jetty is an ordinary graph dependency already excluded
centrally in the root pom - the same arrangement `pinot-hdfs` has always used,
and what `parquet-hadoop` itself declares. `hadoop-client-api` is not an
alternative: it holds only `org/apache/hadoop/*` classes and its bytecode makes
169 references to `org/apache/hadoop/shaded/org/eclipse` (Jetty) among 13
relocated packages, so it cannot be used without `hadoop-client-runtime` and
would bring the same Jetty back. The shade filter is retained because the two
external-process batch plugins still shade.
**Plugins `pinot-tools` does not compile against are excluded from the
uber-jar.** `pinot-tools` depends on sixteen plugin modules, and ideally it
would not: the quickstarts and admin commands reach for plugin classes, a few
by import but most only by fully-qualified name at runtime. The distribution
therefore carried those plugins twice - once shaded into `lib/pinot-all.jar`
through `pinot-tools`, once as the plugin itself - and the uber-jar copy wins
on the classpath, making the plugin directory dead weight for exactly those
plugins. `pinot-distribution` now excludes the fourteen `pinot-tools` never
references at compile time; `pinot-avro`, `pinot-json`, `pinot-parquet` and
`pinot-minion-builtin-tasks` stay, because it genuinely imports from them and
removing those needs their shared classes extracted into library modules first.
Declaring those dependencies `provided` in `pinot-tools` would keep the list
next to what it describes and would be the better mechanism, but `provided` is
not on the runtime classpath, so every quickstart needing one of these plugins
would stop working when launched from Maven or an IDE, where there is no
`plugins/` directory for `PluginManager` to load from. Both poms carry that
reasoning.
**Out of scope, deliberately.** `pinot-spi`, `pinot-core`, `pinot-common`
and `pinot-distribution` keep publishing their shaded classifiers, and
`pinot-cli` and `pinot-perf` keep their runnable fat jars. `pinot-spi` and
`pinot-core` in fact already do not shade on a default build - their
`build-shaded-jar` profiles are `activeByDefault=false`. Nothing in the repo
consumes a `<classifier>shaded</classifier>` dependency, so `pinot-common`'s
shaded classifier may well be unnecessary, but these are published artifacts
with external consumers I cannot enumerate. `lib/pinot-all.jar` also stays a
single shaded jar; splitting it deserves its own change.
## Why this does not change runtime behavior
The launcher has always globbed every jar under `plugins/` onto the JVM
classpath, so all plugin classes and all plugin dependencies were already on
one flat classpath, resolved first-match-wins. Splitting a fat jar into a thin
jar plus its dependency jars, all still on that same classpath in the same
position, produces the same set of classes with the same resolution order.
Plugin discovery is unaffected: `PluginManager` finds a plugin by locating a
jar and taking its parent directory name, and the thin jar sits exactly where
the fat jar did.
Relocation removal has one narrow effect. A third-party plugin that bundles
its own *unrelocated* jackson, guava or scala used to get its own copy, because
Pinot's copies lived under `org.apache.pinot.shaded`; it will now resolve
Pinot's copies instead, by classpath order. This is the same exposure such a
plugin already had for every other library, and the standard fix - relocating
your own dependencies in your own fat jar - is unaffected.
One more honest footnote. Where two plugins disagreed on the version of a
library, the fat-jar layout resolved that by whichever plugin jar the classpath
reached first; the shared store resolves it by whichever of the two jars the
`plugin-libs/*` expansion reaches first. Both are arbitrary and neither is
specified, but the winner can change. Auditing the built distribution, exactly
one such case exists: `threeten-extra` 1.7.1 (via `pinot-orc`) against 1.8.0
(via `pinot-gcs`). The other apparent duplicates are not conflicts -
`swagger-annotations` 1.x and 2.x use different packages, `metrics-core` 2.2.0
and 4.2.39 are the unrelated Yammer and Dropwizard artifacts,
`netty-transport-native-epoll` and `jffi` differ only by classifier, and the
three `annotations-*.jar` files are unrelated artifacts from AWS, JetBrains and
gRPC that merely share a file-name stem.
That last case is worth naming as a limitation: the store keys on
`artifactId-version.jar`, so two artifacts from different groups with the same
artifactId *and* the same version would silently overwrite each other. Every
jar in the built store was hashed and no such collision exists; if it ever
becomes a concern the assembly can switch to an `outputFileNameMapping` that
includes the groupId.
## Verification
Measured on one machine, same warm local repository, `-T 1C -P
'bin-dist,!pinot-fastdev'` with the same checks skipped on both sides, against
a distribution built from the same commit without these changes:
| | before | after |
|---|---|---|
| `bin-dist` wall clock | 4:13 | **3:23** (-20%) |
| modules running shade | 37 | **8** |
| distribution size | 1144 MB | **732 MB** (-36%) |
| `lib/pinot-all.jar` | 316 MB | **250 MB** |
| `org.apache.kafka` classes in the uber-jar | 5024 | **0** |
| Jetty classes on the runtime classpath | 0 | **0** |
- **Deduplication**: across the 28 shipped plugins, 1115 dependency-jar
copies reduce to 464 distinct artifacts, eliminating 651 copies (58%).
`slf4j-api` was embedded 15 times, `jackson-annotations` 13, `jackson-databind`
and `jackson-core` 12 each.
- **The shared store is complete and consistent**: all 464 expected
dependency jars are present, nothing missing and nothing extra. Every jar was
hashed - no two plugins contribute the same file name with different content,
so merging them is safe.
- **No classes lost.** Comparing the class sets reachable on the runtime
classpath of both built distributions (`lib/*` + `plugins/**` +
`plugin-libs/*`), everything absent afterwards is accounted for by design: the
relocated `org/apache/pinot/shaded/*` copies, now present at their real
coordinates, and the relocated `org/apache/hadoop/shaded/*` payload of the
dropped uber-jar. Nothing else.
- **Plugin selection still works**, checked against the artifacts the build
produces: `PLUGINS_INCLUDE=pinot-json` yields that plugin's jar plus exactly
its five jackson dependencies; `PLUGINS_INCLUDE=pinot-s3` yields its jar plus
57 AWS jars and no Kafka or Scala jar at all; selecting nothing yields every
plugin jar plus the whole store, as before; and an old-format plugin directory
with no index still contributes its own jars.
- **A real Spark ingestion launch** against the built distribution, with
`SPARK_HOME` pointing at a Spark 3.5.9 `scala2.13` build: the default selection
ships 439 store jars, each traceable to a plugin that declares it, with Scala
correctly withheld because only the excluded `pinot-kafka-3.0` declares it;
`-pluginsToLoad
pinot-batch-ingestion-spark-3:pinot-csv:pinot-batch-ingestion-standalone` ships
6 store jars instead of 439. `SparkSubmit` starts and `PluginManager` loads
plugins from the new layout. Segment generation itself cannot complete on a
Java 25 build for a known unrelated reason (Hadoop 3.4's `UserGroupInformation`
calls `Subject.getSubject`, which throws on JDK 23+).
- **Tests**: `LaunchSparkDataIngestionJobCommandTest` 12 run, 0 failures,
covering the selection rules and the index reader (`:`-separated,
newline-separated, blank, empty and absent). `pinot-parquet` 59 run, 0 failures.
- `quick-start-batch.sh` runs end to end from the built distribution: 15
tables, sample queries return, and no `NoClassDefFoundError`,
`ClassNotFoundException` or plugin-loading failure.
- `quick-start-streaming.sh` was exercised with an external Kafka. The Kafka
plugin loads and drives a real broker - its classes are absent from
`lib/pinot-all.jar` and present only in `plugins/.../pinot-kafka-3.0-*.jar` -
topics are created, and there are no classloading failures. A complete green
run was not obtained in my environment: it needs Docker for its managed Kafka,
and with an external broker the controller timed out validating the
`githubEvents` stream. I could not attribute that to this change and did not
compare it against an unmodified build in the same setup, so I am flagging it
rather than claiming a pass.
Both distributions also hit the same pre-existing `Unable to create index
directory` failure on `dimBaseballTeams_OFFLINE`, a check-then-act race in
`BaseTableDataManager` where `mkdirs()` returns false for the losing thread of
two concurrent state transitions on a dimension table. Unrelated to this PR.
## Backward incompatibility
Labelled `backward-incompat`; for the release notes:
1. `org.apache.pinot.shaded.*` classes are gone from `pinot-all.jar` and
from the plugin jars. Anything compiled against those relocated coordinates
must move to the real coordinates.
2. Plugin jars in the distribution are no longer named `*-shaded.jar`, and a
plugin's dependencies now live in the new top-level `plugin-libs/` directory
rather than inside the plugin jar. Tooling that copies individual plugin jars
out of the tarball must take `plugin-libs/` too. The per-plugin directory
layout under `plugins/` is unchanged.
3. Third-party plugins that bundle unrelocated jackson, guava or scala now
resolve Pinot's copies of those libraries. Relocate them in your own plugin jar
if you need your own versions.
4. `hadoop-client-runtime` is no longer shipped in the distribution.
5. Each plugin directory gains a `pinot-plugin.classpath` file.
`PLUGINS_INCLUDE` and the Spark launcher's `-pluginsToLoad` /
`-pluginsToExclude` keep their meaning; a third-party plugin that ships no
index keeps its current behaviour.
6. `lib/pinot-all.jar` no longer contains the plugin classes and third-party
payload that `pinot-tools` used to drag in.
## Follow-ups
- Classloader-realm isolation - plugin realms, realm-aware service
discovery, the plugin verifier - is the other half of #18459 and is being
rebased onto this change as a separate PR. Note for that work: a realm built
from a plugin directory does not see `plugin-libs/`, so it must build its URL
list from the plugin's `pinot-plugin.classpath` rather than from
`Files.list(pluginDir)`. The index this PR adds is exactly what it needs; the
mistake to avoid is adding `pinot-plugin.properties` files without consuming
it, which would leave every realm-loaded plugin without its dependencies.
- `lib/pinot-all.jar` could dissolve into the same shared-store model,
removing the largest remaining shade execution and most of the remaining
duplication. Measured on this build: the core runtime closure is 419 jars and
`plugin-libs/` is 464, of which 337 are the same artifact - an 80% overlap.
Today that costs 316 MB plus 329 MB; the union of both as loose jars is 532
jars totalling 390 MB, so roughly 255 MB is recoverable. The launcher needs no
change since it already globs `lib/*`. The compat question is that
`pinot-all.jar` is matched by name by `LaunchSparkDataIngestionJobCommand` and
is what users pass to `spark-submit`, so the tarball would stop using a single
merged jar while `pinot-distribution` keeps publishing one.
- A tempting shortcut to avoid: 306 of the 464 jars in `plugin-libs/` (237
MB) are fully shadowed by `lib/pinot-all.jar` and so are never resolved from
the store, which makes omitting them look free. It is not. Much of that
shadowing existed only because `pinot-tools` pulled plugin modules into the
uber-jar, which this PR largely removes; it would couple the plugin layout to
whatever the uber-jar happens to contain; and a plugin loaded in a strict realm
never sees `lib/` at all, so its dependencies must really be present.
- `pinot-common`'s shaded classifier appears to have no consumers and could
be dropped.
- The remaining four compile-time plugin dependencies of `pinot-tools` could
be removed by extracting their shared classes into `-base` library modules,
which would take the last plugin classes out of `pinot-all.jar`.
--
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]
