xiangfu0 opened a new pull request, #19262:
URL: https://github.com/apache/pinot/pull/19262
> **Stacked on #19259** — this branch includes that PR's commit; only the
second commit is new here. Will rebase once #19259 merges.
## Summary
Follow-up to #19259. Three call sites hand-rolled the same ServiceLoader
enumeration pattern — load from the thread context classloader, then from every
classloader returned by `PluginManager.get().getPluginClassLoaders()`,
de-duplicating providers by fully-qualified class name:
- `PinotRuleSet.loadFromServiceLoader()` (pinot-query-planner)
- `OperatorTypeRegistry` static initializer (pinot-query-runtime)
- `TransformFunctionFactory.registerServiceProviders(...)` (pinot-core)
This PR extracts the shared mechanism into pinot-spi as
`PluginManager#loadServiceProviders(Class)` and migrates all three call sites.
Any future fix to the enumeration mechanism (dedup semantics, realm ordering,
error context) now lands in one place.
## The helper
```java
List<PluginManager.ServiceProvider<MyService>> providers =
PluginManager.get().loadServiceProviders(MyService.class);
```
- Enumerates the thread context classloader first (the application classpath
in a standard deployment), then every plugin classloader, in load order.
- De-duplicates providers by fully-qualified class name across classloaders
(overlapping classpaths — e.g. fat-jar + plugin realm — surface the same
provider through multiple loaders; the first sighting wins). A same-named
provider whose `Class` object differs from the first sighting indicates version
skew between classloaders and is logged at WARN before being skipped.
- Each returned `ServiceProvider<S>` pairs the provider instance with a
human-readable source description for call-site log/error messages.
- Enumeration failures (`ServiceConfigurationError` — malformed descriptors,
missing/incompatible/unconstructable provider classes) fail fast as
`IllegalStateException` carrying the source description with the original error
preserved as the cause.
- Per-provider validation and registration policy stay with the caller
(id-floor checks in `OperatorTypeRegistry`, name canonicalization/collision
rules in `TransformFunctionFactory`, ordering in `PinotRuleSet`).
## New public SPI surface (reviewer attention)
This adds a permanent additive contract to pinot-spi that plugin authors can
depend on: `PluginManager#loadServiceProviders(Class)` and the nested immutable
`PluginManager.ServiceProvider<S>` pair type, with these semantics:
context-classloader-first discovery order, first-sighting-wins dedup by class
name, WARN-and-skip on classloader version skew, an unmodifiable freshly
computed result list, and fail-fast `IllegalStateException` wrapping of
`ServiceConfigurationError` with source context. No existing signature changed;
`getPluginClassLoaders()` remains for callers needing the raw classloaders.
## Behavior notes
- `TransformFunctionFactory`: no behavior change — it previously used the
exact logic now hosted in the helper.
- `PinotRuleSet` / `OperatorTypeRegistry` (deliberate unification onto the
`TransformFunctionFactory` fail-fast contract): a malformed descriptor or
failing provider constructor now surfaces as `IllegalStateException` with
classloader-source context and the `ServiceConfigurationError` as cause,
instead of the raw error propagating with no context (from
`OperatorTypeRegistry`'s static initializer both variants surface as
`ExceptionInInitializerError`). Both paths failed startup/class-init before and
still do; the failure just carries more information. Their silent same-FQCN
dedup now WARNs when the duplicate is a genuinely different class (version
skew). Error precedence can also shift: providers are fully enumerated before
caller-side validation runs, so an enumeration failure from a later classloader
may now surface before a caller-side validation error that previously fired
first — either way startup fails.
- All three modules already depended on pinot-spi; no layering change.
## Testing
- New `PluginManagerServiceProviderTest` (pinot-spi): context-classloader
discovery, plugin-realm discovery, cross-classloader dedup, discovery order,
malformed-descriptor and constructor-failure wrapping (source + cause
asserted), empty result. Descriptors are generated into temp dirs behind
isolated `URLClassLoader`s and fresh `PluginManager` instances, so no fixture
leaks into the test classpath.
- Existing suites pass unchanged: `TransformFunctionFactoryTest` (23),
`ServerInstanceTransformFunctionTest` (3), `PinotRuleSetTest`,
`OperatorTypeRegistryTest`, `PluginRealmExportTest`.
--
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]
