Copilot commented on code in PR #19259:
URL: https://github.com/apache/pinot/pull/19259#discussion_r3810339034
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java:
##########
@@ -503,6 +502,93 @@ public synchronized Set<ClassLoader>
getPluginClassLoaders() {
return Collections.unmodifiableSet(result);
}
+ /// Discovers service providers of the given service type via
`ServiceLoader`, first from the thread context
+ /// classloader (the application classpath in a standard deployment), then
from every plugin classloader (see
+ /// [#getPluginClassLoaders()]), in that order.
+ ///
+ /// Providers are de-duplicated by fully-qualified class name across
classloaders: overlapping classpaths (e.g.
+ /// fat-jar + plugin realm) can surface the same provider through multiple
loaders, and the first sighting wins.
+ /// A same-named provider whose `Class` object differs from the first
sighting indicates version skew between
+ /// classloaders; it is logged at WARN and skipped.
+ ///
+ /// Each returned entry pairs the provider instance with a human-readable
description of the classloader it was
+ /// discovered from, for use in call-site log and error messages.
Enumeration failures
+ /// ([ServiceConfigurationError] — malformed descriptors, provider classes
that cannot be found, are not subtypes
+ /// of the service type, or fail to construct) 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.
+ ///
+ /// Returns an unmodifiable, freshly computed list — never null, empty when
no providers are found. Thread-safe:
+ /// each call performs a fresh enumeration using only method-local state
(the plugin classloader snapshot is taken
+ /// under the existing lock), and the result reflects the calling thread's
context classloader.
+ ///
+ /// Call after all plugins have been loaded; plugin classloaders registered
after this call returns are not
+ /// searched.
+ public <S> List<ServiceProvider<S>> loadServiceProviders(Class<S>
serviceClass) {
+ List<ServiceProvider<S>> providers = new ArrayList<>();
+ Map<String, Class<?>> seenProviderClasses = new HashMap<>();
+ collectServiceProviders(ServiceLoader.load(serviceClass), serviceClass,
"thread context classloader", providers,
+ seenProviderClasses);
+ for (ClassLoader pluginClassLoader : getPluginClassLoaders()) {
+ collectServiceProviders(ServiceLoader.load(serviceClass,
pluginClassLoader), serviceClass,
+ "plugin classloader: " + pluginClassLoader, providers,
seenProviderClasses);
+ }
+ return Collections.unmodifiableList(providers);
+ }
+
+ private static <S> void collectServiceProviders(ServiceLoader<S>
serviceLoader, Class<S> serviceClass, String source,
+ List<ServiceProvider<S>> providers, Map<String, Class<?>>
seenProviderClasses) {
+ Iterator<S> iterator = serviceLoader.iterator();
+ while (true) {
+ S provider;
+ try {
+ if (!iterator.hasNext()) {
+ return;
+ }
+ provider = iterator.next();
+ } catch (ServiceConfigurationError e) {
+ throw new IllegalStateException(
+ "Failed to load a " + serviceClass.getName() + " service provider
from: " + source, e);
+ }
+ Class<?> providerClass = provider.getClass();
+ String providerClassName = providerClass.getName();
+ Class<?> seenClass = seenProviderClasses.putIfAbsent(providerClassName,
providerClass);
+ if (seenClass != null) {
+ // Same provider already discovered through an overlapping
classloader. A different Class object with the
+ // same name indicates version skew between classloaders; the first
discovered copy wins.
+ if (seenClass != providerClass) {
+ LOGGER.warn("Ignoring duplicate service provider class: {}
(classloader: {}, discovered from: {}); keeping "
+ + "the copy from classloader: {}", providerClassName,
providerClass.getClassLoader(), source,
+ seenClass.getClassLoader());
+ }
+ continue;
+ }
Review Comment:
`iterator.next()` constructs each provider before the class-name check
below. Consequently, overlapping realms still run duplicate constructors, and a
version-skewed duplicate whose constructor fails aborts startup instead of
being WARNed and skipped under the documented first-sighting-wins contract.
Enumerate `ServiceLoader.Provider`s, de-duplicate on `type()`, and invoke
`get()` only for unseen types; a stateful/failing duplicate regression test
would cover this.
--
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]
