gortiz commented on PR #18459:
URL: https://github.com/apache/pinot/pull/18459#issuecomment-4429183628
# Review: PR #18459 — Replace global shading with classloader-realm
isolation for plugins
**Repo:** apache/pinot
**Branch:** pinot-no-shading → master
**Author:** gortiz
**Date:** 2026-05-11
**Repo context loaded:**
- [x] CLAUDE.md read (present, comprehensive)
- [x] CODEOWNERS read (not present)
- [x] Module-level docs for touched paths read (N/A — no per-module
CLAUDE.md in touched paths)
---
## Stage 1: Triage & Evidence Collection
### PR description
- [x] Description present
- [x] States what problem is being solved (four problems enumerated: plugin
code baked into main jar, startup script loading plugins onto system classpath,
shading prefix collisions, fragmented extension-point discovery)
- [x] States why this approach (classloader-realm isolation is the right
semantic for plugin isolation; shading is kept only for driver libs and
external-process connectors)
- [x] States risky areas (backward compatibility section, `@MetricsFactory`
annotation change noted, external plugin authors migration path described)
**Findings:**
The description is thorough and accurate. One gap: the description does not
mention that `pinot-kafka-4.0` is intentionally **not** converted (no
`pinot-plugin.properties`, no plugin-zip entry in distribution). This omission
is a potential source of confusion for reviewers and operators. Severity:
observation. Confidence: high. Evidence:
`pinot-plugins/pinot-stream-ingestion/pinot-kafka-4.0/` — no
`src/main/resources` directory exists on the branch; not listed in
`pinot-distribution/pom.xml` plugin deps.
### Change summary
- [x] Full diff read (GitHub API reports 100 files; actual branch diff
against master is 116 files — 16 files absent from the GitHub PR files list
including the most critical ones)
- [x] Summary produced
- [x] Files listed by module
**Summary:**
This is a large architectural refactor with three coordinated phases: (1)
replace `ServiceLoader.load(Iface.class)` call-sites with
`PluginManager.get().loadServices(Iface.class)` across 9+ modules; (2) add
`pinot-plugin.properties` to every plugin module under `pinot-plugins/` and
`pinot-connectors/` so they can be discovered as classloader realms; (3)
replace shaded fat-jar plugin layout in the distribution with plugin-zip layout
(classes + dep jars side-by-side), removing global relocation from the root
`pom.xml`. New tooling: `pinot-plugin-verifier` standalone CLI. New library
modules: `pinot-json-base`, `pinot-parquet-base`.
**Files by module:**
- `pinot-spi/`: `PluginManager.java` (core new logic: `loadServices`,
`loadClassFromAnyPlugin`, jar-resident `pinot-plugin.properties` discovery),
`PinotMetricUtils.java`, `ExecutorServiceUtils.java`,
`ResponseStoreService.java`, `RecordEnricherRegistry.java`,
`PluginManagerTest.java` (+ new test SPI fixtures)
- `pinot-segment-spi/`: `IndexService.java`
- `pinot-query-runtime/`: `OpChainConverterDispatcher.java`
- `pinot-broker/`: `AccessControlFactory.java`, new
`AccessControlFactoryLoaderTest.java`
- `pinot-common/`: `pom.xml` (remove shade profile),
`LogicalTableConfigSerDeProvider.java`, `FakeMetricsFactory.java`
- `pinot-core/`: `pom.xml` (remove shade profile),
`TimeBoundaryStrategyService.java`
- `pinot-distribution/`: `pom.xml` (plugin-zip deps + shade execution
override), `pinot-assembly.xml` (complete rewrite from file-list to
dependencySet unpack)
- `pinot-plugins/`: ~30 `pom.xml` files (`shade.phase.prop` removal,
`plugin.type` removal), ~25 new `pinot-plugin.properties` files; new
`assembly-descriptor/` with `pinot-plugin.xml` assembly descriptor; new
`pinot-json-base/`, new `pinot-parquet-base/`; compound-metrics
`CompoundPinotMetricsFactory.java`
- `pinot-plugin-verifier/`: entirely new module (`PluginVerifier.java`,
check classes, smoke test)
- `pinot-minion/`: `SegmentConversionUtils.java` (moved from
`pinot-minion-builtin-tasks` to `pinot-minion`)
- `pinot-tools/`: `LaunchBackfillIngestionJobCommand.java` (import update
for moved class)
- `pinot-clients/`: both client `pom.xml` files (explicit shade relocations
added)
- Root `pom.xml`: remove global shade relocations, add
`pinot-plugin-verifier` module, add `pinot-json-base` and `pinot-parquet-base`
to `<dependencyManagement>`
**NOTE:** The GitHub PR files API omits 16 files from the diff, including
`PluginManager.java`, `PinotMetricUtils.java`, `IndexService.java`,
`ResponseStoreService.java`, `ExecutorServiceUtils.java`,
`OpChainConverterDispatcher.java`, `RecordEnricherRegistry.java`, root
`pom.xml`, and several test files. These were retrieved by fetching the branch
directly. Reviewers relying only on the GitHub diff UI will miss the most
critical code changes in this PR.
### Risk classification
- [x] Classified as **high**
- [x] Reason stated
**Classification:** HIGH
**Reason:** This PR modifies the plugin loading subsystem (PluginManager —
the central classloader registry for all Pinot extension points), changes how
every `ServiceLoader`-based extension point is discovered at startup across
broker/server/controller/minion, rewrites the distribution packaging layout for
all plugins, removes global shading relocations from the root POM (affecting
every module that inherits the shade plugin configuration), and introduces a
new module ordering dependency. A failure in any of these areas breaks every
Pinot service role at startup. The blast radius is the entire cluster.
### Test gaps
- [x] Changed behaviors/contracts checked for corresponding tests
- [x] Test coverage of new/changed behavior verified
**Gaps:**
1. **`loadServices` deduplication across multiple classloaders not tested.**
The code comments say cross-classloader FQCN deduplication is intentional but
"out of scope for this unit test." However this is exactly the scenario that
matters in production: a class reachable via the DEFAULT classloader AND a
plugin realm. Without a test, the correctness of `seenFqcns` de-duplication
under the real multi-classloader layout is unverified. Severity: concern.
Confidence: medium. Evidence:
`PluginManagerTest.testLoadServicesIsIdempotentAcrossInvocations` comment
explicitly says the cross-classloader dedup test "is out of scope."
2. **`readPluginConfigurationFromJar` startup cost untested.** The
jar-scanning fallback opens every JAR in a plugin directory to look for
`pinot-plugin.properties`. Kafka-3.0 ships 150+ JAR files in its zip. No
benchmark or test validates acceptable startup time under the new layout.
Severity: concern. Confidence: medium. Evidence:
`PluginManager.readPluginConfiguration()` iterates
`FileUtils.listFiles(directory, new String[]{"jar"}, true)` opening each with
`new JarFile(jarFile)`.
3. **No integration test for the complete plugin-zip → realm load → class
instantiation path in CI.** The `PluginVerifierSmokeTest` explicitly
acknowledges it cannot test the real production layout; the end-to-end test
(`quick-start-batch.sh`) is manual in the test plan. Severity: concern.
Confidence: high. Evidence: `PluginVerifierSmokeTest` class Javadoc: "We don't
have a built distribution at unit-test time, so these only confirm that the
binary is wired together correctly."
4. **`AccessControlFactory` test does not verify plugin-realm-resident class
loading.** The test only covers system-classloader-resident classes. Severity:
observation. Confidence: high. Evidence: `AccessControlFactoryLoaderTest` — all
tested FQCNs are in-tree classes visible on the test classpath.
---
## Stage 2: Deep Review
### Does the implementation address the stated problem?
- [x] Problem statement identified from the PR description
- [x] Implementation traced through the code to check whether it appears to
address the stated problem
- [x] No obvious gaps between what the PR claims to fix and what the code
actually does
**Findings:**
All four stated problems have corresponding implementation changes:
1. Plugin code baked into main service jar — addressed: global shade
relocations removed from root `pom.xml`, plugins now produce zip artifacts
instead of shaded fat-jars.
2. Startup script putting plugin jars on the JVM classpath — the assembly
descriptor changes remove the `<file>` entries for shaded plugin jars, so the
distribution no longer places them where the startup script can find them.
However, the startup script (`appAssemblerScriptTemplate`) itself is not
modified in this PR; whether it still walks `plugins/**/*.jar` onto `CLASSPATH`
is not verified in the diff.
3. Shading prefix collisions — addressed by removing relocations from the
root `pom.xml`.
4. Fragmented extension-point discovery — addressed by the `loadServices`
helper replacing every `ServiceLoader.load(X.class)` call-site across the
codebase.
Severity: observation. Confidence: medium. Evidence: startup script not in
changed files; `appAssemblerScriptTemplate` behavior needs manual verification.
### High-risk files
- [x] Files ranked by logical risk
- [x] Risk reasons stated per file
| Rank | File | Risk reason |
|---|---|---|
| 1 | `pinot-spi/.../PluginManager.java` | New `loadServices`,
`loadClassFromAnyPlugin`, jar-scan fallback affect every plugin load at startup
|
| 2 | `pom.xml` (root) | Removing global shade relocations changes every
module's shading behavior |
| 3 | `pinot-distribution/pinot-assembly.xml` | Complete packaging rewrite;
assembly failure = broken distribution |
| 4 | `pinot-distribution/pom.xml` | New plugin-zip dependency declarations
+ shade execution override |
| 5 | `pinot-spi/.../PinotMetricUtils.java` | Metrics factory discovery
change affects startup of every service role |
| 6 | `pinot-plugins/assembly-descriptor/` | New shared assembly descriptor
drives plugin zip production; bugs here break all plugin zips |
**Findings:** The highest-risk file (`PluginManager.java`) is absent from
the GitHub PR files API list, making the GitHub diff UI an incomplete review
surface for this PR. Severity: concern. Confidence: high. Evidence: `gh pr view
18459 --repo apache/pinot --json files` returns 100 files; `git diff
origin/master...personal/pinot-no-shading` produces 116 file diffs.
### Center of gravity
- [x] 1-3 key files or decisions identified
**Files:**
1. `pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java`
2.
`pinot-plugins/assembly-descriptor/src/main/resources/assemblies/pinot-plugin.xml`
3. `pom.xml` (root) — shade relocation removal
**Why these are central:**
- `PluginManager.java` is the runtime mechanism: the `loadServices` method
is called at startup from metrics, index, executor, record-enricher,
response-store, time-boundary, op-chain, and access-control subsystems.
Correctness here determines whether every plugin-based feature works.
- `pinot-plugin.xml` assembly descriptor determines the zip layout that
`PluginManager.readPluginConfiguration` and the distribution assembly depend
on. If the layout is wrong, no plugin is ever loaded as a realm.
- Root `pom.xml` shade relocation removal is the one change that could
silently break existing deployments: any module that previously worked because
it consumed relocated Jackson/Guava from `org.apache.pinot.shaded.*` would fail
at runtime if it now gets the unshaded version (or nothing).
### Invariants — targeted
- [x] Authorization — boundaries preserved; `AccessControlFactory` change
uses `PluginManager.loadClass` which delegates to system classloader first,
preserving current behavior for in-tree factories.
- [x] Idempotency — `loadServices` is not idempotent (each call does a fresh
walk), documented as intentional.
- [x] Backward compatibility — APIs: no wire protocol changes.
- [x] Backward compatibility — Schemas: not applicable; no schema changes.
- [x] Backward compatibility — SPI: `PluginManager.loadClass`,
`createInstance`, `loadServices` are new public APIs; existing
`loadClass(pluginName, className)` behavior is changed (now does realm walk for
DEFAULT_PLUGIN_NAME). This is additive but changes the resolution order for
bare-FQCN lookups.
- [x] Backward compatibility — Configuration: `pinot-plugin.properties`
files added to existing plugins. The root `pom.xml` shade relocation removal
changes the shading behavior for any module that inherited it.
- [x] Ordering guarantees — `loadServices` iteration order is documented
(PluginManager classloader first, then ClassWorld realms in registration order,
then legacy PluginClassLoaders). This is deterministic but depends on plugin
load order, which depends on filesystem iteration (directory listing order).
- [x] Null/empty handling — `loadClassFromAnyPlugin` throws
`ClassNotFoundException` cleanly when no classloader finds the class.
`loadServices` returns an empty list when no implementation is found. Both are
correct.
- [x] Retry safety — not applicable; no retry logic changed.
- [x] Observability — `LOGGER.warn` added for FQCN conflicts across
classloaders and malformed service entries. `LOGGER.info` logs jar-resident
property file discovery.
- [x] Resource cleanup — `readPluginConfigurationFromJar` uses
try-with-resources on `JarFile`. No resource leaks found.
- [x] Migration reversibility — removing global shade relocations is NOT
reversible without a coordinated release because any consumer that already
depends on `org.apache.pinot.shaded.*` class names (e.g. in configuration)
would break if shading is re-added.
- [x] Performance in hot paths — `loadServices` is called at startup, not on
hot paths. However, `readPluginConfiguration` opens all JARs in a plugin
directory recursively on startup to scan for `pinot-plugin.properties` — this
adds meaningful startup I/O for plugins with many dependencies (see Test gaps
#2).
- [x] API design — `loadServices` return type is `List<T>` (not
`Iterable<T>`), which is a reasonable choice. The `loadClassFromAnyPlugin`
first-hit-wins semantics is clearly documented. The `synchronized (this)`
snapshot approach is sound.
**Findings:**
BLOCKER 1 — `verify-plugins.sh` is referenced in `pinot-assembly.xml` but
does not exist in the repository. The distribution build will fail at assembly
time when Maven tries to include
`${pinot.root}/pinot-plugin-verifier/src/main/resources/bin/verify-plugins.sh`
because the file does not exist. Severity: blocker. Confidence: high. Evidence:
`pinot-distribution/pinot-assembly.xml` line includes
`<source>${pinot.root}/pinot-plugin-verifier/src/main/resources/bin/verify-plugins.sh</source>`;
`git ls-tree
personal/pinot-no-shading:pinot-plugin-verifier/src/main/resources/` returns no
`bin/` directory.
BLOCKER 2 — `pinot-plugin-verifier` pom.xml produces a plain `.jar` (no
`maven-assembly-plugin` config, no manifest main class, no
`jar-with-dependencies` classifier), but the PR test plan instructs users to
run `java -jar
pinot-plugin-verifier/target/pinot-plugin-verifier-*-jar-with-dependencies.jar`
and the assembly.xml includes the plain `.jar`. The artifact referenced in the
test plan does not exist and the plain `.jar` lacks a runnable manifest.
Severity: blocker. Confidence: high. Evidence: `pinot-plugin-verifier/pom.xml`
— no `maven-assembly-plugin` or `maven-jar-plugin` `<mainClass>` config; PR
test plan line: `java -jar
pinot-plugin-verifier/target/pinot-plugin-verifier-*-jar-with-dependencies.jar`.
CONCERN 1 — `pinot-kafka-4.0` has no `pinot-plugin.properties` and is not
included in the distribution's plugin-zip list. If Kafka 4.x is a supported
stream ingestion plugin, it will silently not load as a classloader realm under
the new layout. Any deployment that uses `pinot-kafka-4.0` would break.
Severity: concern. Confidence: medium (need to verify whether kafka-4.0 is in
active use). Evidence: `git ls-tree
personal/pinot-no-shading:pinot-plugins/pinot-stream-ingestion/pinot-kafka-4.0/src/main/`
shows only `java/`; `pinot-distribution/pom.xml` has no `pinot-kafka-4.0`
dependency.
CONCERN 2 — Backward-compatibility break for `@MetricsFactory` annotation.
The PR description states: "The `@MetricsFactory` annotation is preserved but
is no longer load-bearing; plugins must add
`@AutoService(PinotMetricsFactory.class)` (or hand-write a service file) to be
discovered." This is a **breaking change for third-party plugins** that rely on
`@MetricsFactory` for discovery. The PR provides no migration path or
grace-period warning in `PinotMetricUtils`. Severity: concern. Confidence:
high. Evidence: `PinotMetricUtils.initializePinotMetricsFactory` now calls
`PluginManager.get().loadServices(PinotMetricsFactory.class)` which requires a
`META-INF/services` entry; `@MetricsFactory` scanning via
`PinotReflectionUtils.getClassesThroughReflection` is removed entirely.
CONCERN 3 — The assembly `dependencySet` unpacks all plugin zips into a
single flat `plugins/` directory. Each plugin zip's `pinot-plugin.xml`
descriptor puts its contents under `${project.artifactId}/` inside the zip
(e.g., `pinot-kafka-3.0/classes/`, `pinot-kafka-3.0/*.jar`). After unpacking,
the plugins directory will contain subdirectories named after artifact IDs
(e.g., `plugins/pinot-kafka-3.0/`). The `PluginManager` scans
`<pluginsDir>/<subdirName>/` using `<subdirName>` as the plugin name. Verifying
that this directory structure matches what `PluginManager.load(pluginName,
directory)` expects requires an end-to-end build; it is not verifiable from the
diff alone. Severity: concern. Confidence: medium. Evidence: `pinot-plugin.xml`
assembly descriptor + `pinot-assembly.xml` dependencySet
`<outputDirectory>plugins</outputDirectory>` + `PluginManager.load(String
pluginName, File directory)`.
CONCERN 4 — Recursive JAR scan on startup. `readPluginConfiguration()` calls
`FileUtils.listFiles(directory, new String[]{"jar"}, true)` which recursively
scans all JAR files in the plugin directory, then opens each one with `new
JarFile(jarFile)` until a `pinot-plugin.properties` is found. A plugin with
100+ dependency JARs (e.g., `pinot-hdfs`) will cause 100 `JarFile` open
operations per plugin on every startup. The optimization (check the primary
plugin JAR first) helps but does not eliminate the worst case. Severity:
concern. Confidence: high. Evidence: `PluginManager.readPluginConfiguration`
iterates over all JARs recursively; `pinot-plugin-verifier/README.md` mentions
"100+ jars per plugin."
### Invariants — full sweep
- [x] Every invariant above checked regardless of whether the touched paths
trigger it
**Findings:**
ADDITIONAL OBSERVATION — `loadServices` is not thread-safe in its iteration
phase. The snapshot of `classLoaders` is taken under `synchronized (this)`, but
the iteration over `classLoaders` (and the `ServiceLoader` calls inside
`loadServicesInto`) happens outside the lock. This is documented as intentional
("point-in-time snapshot"). The scenario where a plugin is registered AFTER the
snapshot but BEFORE the iteration would miss that plugin's services. This is
the correct trade-off (avoid holding the lock during potentially slow I/O), but
it means callers in early-init code could miss services if another thread
registers plugins concurrently. Severity: observation. Confidence: medium.
Evidence: `PluginManager.loadServices` — `synchronized` block ends before the
`for (ClassLoader cl : classLoaders)` loop.
ADDITIONAL OBSERVATION — `loadClassFromAnyPlugin` calls `Class.forName(name,
true, cl)` with `initialize=true`. If a class triggers its static initializer
upon loading and that initializer has side effects (common in plugin code),
those side effects execute during the walk even if the class is ultimately not
the "winning" hit. This is the same behavior as before for the DEFAULT
classloader path but is now triggered across all classloaders. Severity:
observation. Confidence: medium. Evidence: `loadClassFromAnyPlugin` line:
`Class<?> c = Class.forName(name, true, cl)`.
### Test adequacy
- [x] Tests prove intended behavior (not just that code runs)
- [x] Failure modes covered
- [x] Boundary conditions covered
- [x] Not over-coupled to implementation details
**Findings:**
- `PluginManagerTest.testRealmWalkContinuesPastNoClassDefFoundError`
correctly tests the NCDFE bug fix by injecting a bad classloader via
reflection. This is good.
- `PluginManagerTest.testLoadServicesFindsImplementationsViaServiceLoader`
validates the system-classloader path but does not test the realm path (no
realm is populated in the test).
-
`AccessControlFactoryLoaderTest.loadFactory_withPackagePrivateConstructor_succeeds`
is an excellent regression test for the historical constructor-visibility
contract.
- The smoke test uses `System.setOut/setErr` without restoring on exception
in all paths — the finally block restores them, so this is fine.
- Test plan checklist items for `quick-start-batch.sh` and `PluginVerifier`
21/21 PASS are listed as unchecked boxes, meaning they have not been verified
as part of this PR submission.
Severity: concern. Confidence: high. Evidence: Test plan in PR body shows
unchecked boxes for the most important end-to-end validations.
### Collateral damage
- [x] No copy-paste duplication introduced
- [x] No partial renames (all references updated — `SegmentConversionUtils`
moved and all callers updated)
- [x] No dead code left behind
- [x] Docs and config match code changes — **EXCEPTION:
`pinot-plugin-verifier/README.md` references `java -jar
...-jar-with-dependencies.jar` but that artifact is not produced**
- [x] No one-off exceptions added to shared abstractions
**Findings:** The README/test-plan mismatch about the verifier jar artifact
name is a collateral damage issue. Severity: blocker (see Blocker 2 above).
### Data flow
- [x] Input paths traced from entry point to storage/output
- [x] Unvalidated input paths flagged
- [x] Sensitive data exposure checked
**Findings:**
The `readPluginConfigurationFromJar` method reads untrusted file content
(properties loaded from a JAR file on the filesystem). Properties are parsed
with `java.util.Properties.load()` which is safe (no code execution). The
resulting `PinotPluginConfiguration` is used to configure classloader parent
relationships — a malicious or corrupted `pinot-plugin.properties` could
configure an unexpected `parent.realmId`, but this is bounded by the realm
names already registered in `ClassWorld`. No sensitive data exposure found.
The `loadClassFromAnyPlugin` first-hit-wins semantics means a plugin that
ships a class with the same FQCN as a core class will have that class loaded
from the plugin if the plugin's realm is checked before the system classloader
(in the current ordering: system CL comes first via DEFAULT PluginClassLoader,
so core classes are safe). Severity: observation. Confidence: high.
### Duplicated logic
- [x] PR checked for logic that already exists elsewhere in the codebase
**Findings:**
`ExecutorServiceUtils` previously had a custom `forEachExecutorThatLoads`
that handled `ServiceConfigurationError` recovery. The new
`PluginManager.loadServicesInto` implements the same pattern generically. The
old custom method is deleted, so this is correct consolidation with no
duplication. No other duplicated logic found.
### Path-triggered review
- [x] Touched paths listed
- [x] Failure mode inferred per path
- [x] Reviewed for inferred risks
- [x] Boundary areas checked first
**Touched paths and inferred risks:**
| Path | Inferred failure mode | What was checked | Findings |
|---|---|---|---|
| `pinot-spi/...PluginManager.java` | Blast radius: class loading for all
plugins/services | `loadServices`, `loadClassFromAnyPlugin` logic,
synchronization, thread safety | See Concern 4, Observations on
`Class.forName(name, true, cl)` |
| `pinot-spi/...PinotMetricUtils.java` | Startup failure: metrics factory
not found → no metrics → service won't start | Discovery path: `loadServices` →
`matched.ifPresent` → `init` | Concern 2: `@MetricsFactory` no longer
load-bearing — breaking for external plugins |
| `pinot-distribution/pinot-assembly.xml` | Build failure: assembly
references missing file | `verify-plugins.sh` source path | BLOCKER 1: file
does not exist |
| `pinot-distribution/pom.xml` | Build failure: plugin zips not produced,
shade phase override | Shade execution override + plugin-zip deps | Correct —
shade execution hardcoded to `package` phase |
| `pinot-plugins/assembly-descriptor/` | All plugin zips wrong layout |
`pinot-plugin.xml` descriptor — zip structure | Appears correct; structure
matches `PluginManager.load` expectations |
| `pom.xml` (root) | Runtime failure: classes previously accessed as
`org.apache.pinot.shaded.*` | Global relocation removal | Concern — third-party
plugins that hardcode shaded package names would break |
| `pinot-plugins/pinot-stream-ingestion/pinot-kafka-4.0/` | Silent:
kafka-4.0 not loaded as realm | No `pinot-plugin.properties`, not in
distribution | Concern 1 |
| `pinot-minion/.../SegmentConversionUtils.java` | Package-move: callers
must update imports | All callers updated
(`BaseMultipleSegmentsConversionExecutor`,
`BaseSingleSegmentConversionExecutor`, `SegmentConversionUtilsTest`,
`LaunchBackfillIngestionJobCommand`) | Clean |
| `pinot-clients/pinot-java-client/pom.xml`, `pinot-jdbc-client/pom.xml` |
Shade regression: client jars must still relocate deps | Explicit relocation
config added | Correct; relocations moved from inherited global to module-local
|
**Findings:** Summary of path-triggered findings is incorporated into the
main Blockers/Concerns sections.
### Evidence
- [x] Required evidence checked
| Evidence type | Present? | Notes |
|---|---|---|
| Before/after benchmarks | No | No startup-time benchmarks for jar-scan
fallback |
| Failing test that now passes | Partial |
`testRealmWalkContinuesPastNoClassDefFoundError` is a new regression test, not
one converted from failing |
| Rollout plan with stages | No | This is a build-time change; no phased
rollout described |
| Migration notes and rollback steps | No | No rollback steps described;
removing shade relocations is not trivially reversible |
| Example input/output pairs | No | |
| Screenshot, trace, or log snippet | No | "21/21 passes" claimed but not
shown |
| Proof of backward compatibility | No | External plugin compat promised but
not demonstrated |
| Canary or feature flag plan | No | Merge is all-or-nothing |
**Missing:** The PR description claims 21/21 passes on the built
distribution but no evidence is provided. External plugin backward
compatibility is asserted but not demonstrated. No startup-time benchmark.
Severity: concern. Confidence: high. Evidence: PR test plan checkboxes are
unchecked.
---
## Verdict
**Risk:** high
**Verdict:** changes-requested
**Verdict confidence:** high
### Confidence reasoning
The review benefited from full access to the branch (fetched
`personal/pinot-no-shading` locally) which revealed 16 files not in the GitHub
PR diff API, including `PluginManager.java` — the most critical file. The two
blockers are concrete and reproducible from the diff alone. The concerns are
supported by code-level evidence. The main uncertainty is Concern 1 (kafka-4.0
intentional omission vs. oversight) and whether the startup script was
separately updated outside this PR.
### Blockers
**BLOCKER 1 — Missing `verify-plugins.sh` script breaks the distribution
build.**
Severity: blocker. Confidence: high. Evidence:
`pinot-distribution/pinot-assembly.xml` references
`${pinot.root}/pinot-plugin-verifier/src/main/resources/bin/verify-plugins.sh`;
`git ls-tree
personal/pinot-no-shading:pinot-plugin-verifier/src/main/resources/` confirms
the file does not exist. The Maven assembly plugin will fail with a "file not
found" error when building the distribution tarball under `-Pbin-dist`.
**BLOCKER 2 — `pinot-plugin-verifier` jar artifact mismatch makes the tool
non-functional.**
Severity: blocker. Confidence: high. Evidence:
`pinot-plugin-verifier/pom.xml` has no `maven-assembly-plugin` config to
produce a `jar-with-dependencies` artifact and no `maven-jar-plugin`
`<mainClass>` entry; the PR test plan instructs `java -jar
pinot-plugin-verifier/target/pinot-plugin-verifier-*-jar-with-dependencies.jar`;
`pinot-distribution/pinot-assembly.xml` includes the plain `.jar`. Neither the
test-plan artifact nor a runnable jar is produced by the pom as written.
### Concerns
**CONCERN 1 — `pinot-kafka-4.0` is silently not converted to the realm
layout.**
Severity: concern. Confidence: medium. The plugin has no
`pinot-plugin.properties` and is absent from `pinot-distribution/pom.xml`. If
Kafka 4.x is in active use, deployments that configure it will silently fall
back to legacy `PluginClassLoader` behavior — which may or may not work
depending on whether isolation matters for Kafka 4.x. If it is intentionally
excluded, this must be documented. Evidence:
`pinot-plugins/pinot-stream-ingestion/pinot-kafka-4.0/src/main/` has only
`java/`; no entry in `pinot-distribution/pom.xml`.
**CONCERN 2 — `@MetricsFactory` discovery removed without deprecation grace
period.**
Severity: concern. Confidence: high. Third-party plugins that rely on
`@MetricsFactory` annotation for discovery (without `@AutoService` or a
hand-written `META-INF/services` file) will silently stop being discovered
after this change. `PinotMetricUtils.getPinotMetricsFactoryClasses()` now
returns only plugins discoverable via `ServiceLoader`, which requires the
`META-INF/services` file. Evidence: `PinotMetricUtils.java` diff removes
`PinotReflectionUtils.getClassesThroughReflection(METRICS_PACKAGE_REGEX_PATTERN,
MetricsFactory.class)`.
**CONCERN 3 — Plugin-zip layout correctness not verifiable from diff alone.**
Severity: concern. Confidence: medium. The `pinot-plugin.xml` descriptor
produces a zip with `<artifactId>/classes/` and `<artifactId>/*.jar` layout.
This is unpacked into `plugins/` with
`<outputDirectory>plugins</outputDirectory>`. The
`PluginManager.load(pluginName, directory)` is called with `directory` being
one subdirectory under `plugins/`. Whether the resulting layout matches what
`PluginManager.readPluginConfiguration` expects (specifically the
`pluginPropertiesPath =
directory.toPath().resolve(PINOUT_PLUGIN_PROPERTIES_FILE_NAME)` path) requires
a build to verify. Evidence: `pinot-plugin.xml` puts `pinot-plugin.properties`
at `${project.artifactId}/` inside the zip, which after unpack becomes
`plugins/<artifactId>/pinot-plugin.properties` — this should match
`directory.toPath().resolve("pinot-plugin.properties")` if `directory =
plugins/<artifactId>/`. Appears correct but unverified.
**CONCERN 4 — Startup I/O cost from recursive JAR scanning for
`pinot-plugin.properties`.**
Severity: concern. Confidence: high. On each startup, for every plugin
directory that does NOT have a top-level `pinot-plugin.properties` file,
`readPluginConfiguration` opens every JAR file recursively until it finds the
properties. The optimization (primary JAR checked first by name prefix) helps
in the new zip layout but is insufficient if the primary JAR name doesn't match
the directory name (e.g., versioned or classifier suffixes). For 30+ plugins
each with 100+ deps, this can add seconds to startup. Evidence:
`PluginManager.readPluginConfiguration` — `FileUtils.listFiles(directory, new
String[]{"jar"}, true)` with `new JarFile(jarFile)` per file.
**CONCERN 5 — 16 files missing from GitHub PR diff (metadata gap).**
Severity: concern. Confidence: high. The GitHub PR files API returns 100
files; the actual branch contains 116 changed files. Reviewers using the GitHub
web UI will miss `PluginManager.java`, `PinotMetricUtils.java`,
`IndexService.java`, `ExecutorServiceUtils.java`,
`OpChainConverterDispatcher.java`, `RecordEnricherRegistry.java`,
`ResponseStoreService.java`, root `pom.xml`, and test fixtures — the most
important changes in the PR. This is likely caused by exceeding the GitHub API
files limit (100 files). The PR should be reviewed only against the full branch
diff, not the GitHub UI. Evidence: `gh pr view 18459 --json files | .files |
length` returns 100; `git diff origin/master...personal/pinot-no-shading | grep
"^diff --git" | wc -l` returns 116.
### Observations
1. The `PinotMetricUtils.initializePinotMetricsFactory` now instantiates the
factory via `loadServices` (which calls the no-arg constructor via
`ServiceLoader`) and then calls `instance.init(...)`. Previously, the factory
was discovered as a `Class<?>` and instantiated via
`getDeclaredConstructor().newInstance()`. The new path is cleaner (avoids
double construction) and correct. The `annotation == null` guard added here was
missing in the original.
2. The `AccessControlFactory` migration preserves the historical
`getDeclaredConstructor()` contract (non-public constructors work), with a
detailed comment explaining why. This is correct and the test covers it.
3. `SegmentConversionUtils` package move from `pinot-minion-builtin-tasks`
to `pinot-minion.utils` is logically correct (the class has no plugin-specific
content and belongs in the core minion module). All callers are updated.
4. The `assembly-descriptor` module has an integration test
(`maven-invoker-plugin` ITs) which is a good validation of the descriptor
itself.
5. Dropping the `plugin.type` property from parent POMs and the
`intermediate directory level` from the distribution is a net simplification.
The old layout (`plugins/pinot-stream-ingestion/pinot-kafka-3.0/`) is replaced
by flat `plugins/pinot-kafka-3.0/`. Existing deployments with custom scripts
that relied on the two-level layout will break, but this is a documented
trade-off.
6. `CompoundPinotMetricsFactory.Algorithm.SERVICE_LOADER` now uses
`PluginManager.get().loadServices()` which internally calls `ServiceLoader` on
each classloader. This means `CompoundPinotMetricsFactory` will discover Yammer
and Dropwizard factories if they are in plugin realms — which is the intended
behavior.
7. The PR removes the `pinot-batch-ingestion-spark-3` and
`pinot-batch-ingestion-hadoop` `pinot-plugin.properties` as described, but
`pinot-batch-ingestion-spark-3/pom.xml` adds an explicit shade execution
`<phase>package</phase>` to re-enable shading for that module. This is correct
per the PR description.
### Areas recommended for human focus
1. **Verify the startup script** (`appAssemblerScriptTemplate` or
`pinot-admin.sh`) to confirm it no longer adds `plugins/**/*.jar` to the JVM
classpath. If it still does, realm isolation is a no-op at runtime.
2. **`pinot-kafka-4.0` omission** — confirm whether this is intentional or
an oversight; if intentional, document it prominently.
3. **Third-party plugin compatibility** — review the `@MetricsFactory`-only
plugins in the StarTree distribution (if any) before merging; this change
requires them to add `@AutoService` or a service file.
4. **Plugin-zip layout end-to-end** — do a manual `./mvnw install
-DskipTests -Pbin-dist` build after fixing the blockers and verify
`build/plugins/<name>/pinot-plugin.properties` exists for every converted
plugin.
5. **Startup time regression** — time `PluginManager.init()` before and
after with `pinot-fastdev` disabled and a realistic set of plugins.
6. **The 16 missing files in the GitHub diff** — this PR must be reviewed
against `git diff origin/master...personal/pinot-no-shading`, not the GitHub
web UI. Committers should fetch the branch before approving.
### Status
- [x] All applicable Stage 1 boxes checked
- [x] All applicable Stage 2 boxes checked (high risk tier)
- [x] Verdict committed
- [x] Verdict confidence assigned with reasoning
--
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]
