From a81c6831fa29d7a10a5d5482077b70ef61675dff Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 7 Jul 2022 20:35:57 -0400 Subject: [PATCH 1/6] Fix memory leak --- .../xpack/ilm/ILMImmutableStateHandlerProvider.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java index afd2397b8fb6f..f124b13fb3434 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java @@ -10,16 +10,15 @@ import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; -import java.util.Arrays; import java.util.Collection; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; +import java.util.Collections; +import java.util.List; /** * ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface */ public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider { - private static final Set> handlers = ConcurrentHashMap.newKeySet(); + private static volatile Collection> handlers = Collections.emptyList(); @Override public Collection> handlers() { @@ -27,6 +26,6 @@ public Collection> handlers() { } public static void registerHandlers(ImmutableClusterStateHandler... stateHandlers) { - handlers.addAll(Arrays.asList(stateHandlers)); + handlers = List.of(stateHandlers); } } From 158d0db0afbd333edf4999fd691d455f972b1568 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 8 Jul 2022 14:26:06 -0400 Subject: [PATCH 2/6] Remove static requirement --- .../elasticsearch/action/ActionModule.java | 47 +++++++++++++++++++ .../ImmutableClusterStateHandlerProvider.java | 6 ++- .../java/org/elasticsearch/node/Node.java | 2 + .../ilm/ILMImmutableStateHandlerProvider.java | 21 ++++++--- .../xpack/ilm/IndexLifecycle.java | 10 ++-- 5 files changed, 75 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index 1858d8ab28784..b732a462cc236 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -262,6 +262,9 @@ import org.elasticsearch.gateway.TransportNodesListGatewayStartedShards; import org.elasticsearch.health.GetHealthAction; import org.elasticsearch.health.RestGetHealthAction; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.index.seqno.GlobalCheckpointSyncAction; import org.elasticsearch.index.seqno.RetentionLeaseActions; import org.elasticsearch.indices.SystemIndices; @@ -273,6 +276,8 @@ import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.ActionPlugin.ActionHandler; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.plugins.interceptor.RestInterceptorActionPlugin; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; @@ -416,6 +421,7 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -888,6 +894,47 @@ public void initRestHandlers(Supplier nodesInCluster) { registerHandler.accept(new RestCatAction(catActions)); } + /** + * Initializes the immutable cluster state handlers for Elasticsearch and it's modules/plugins + * + * @param pluginsService needed to load all modules/plugins immutable state handlers through SPI + */ + public void initImmutableClusterStateHandlers(PluginsService pluginsService) { + List> handlers = new ArrayList<>(); + + List pluginHandlerProviders = pluginsService.loadServiceProviders( + ImmutableClusterStateHandlerProvider.class + ); + + // Add directly the handlers that the server has + handlers.add(new ImmutableClusterSettingsAction(clusterSettings)); + + Map, ImmutableClusterStateHandlerProvider> classProviders = new HashMap<>(); + Map> loadedPlugins = new HashMap<>(); + + // Get all plugin handler providers, map the plugin class they need to the provider + for (var pluginHandlerProvider : pluginHandlerProviders) { + pluginHandlerProvider.supportedPlugins().forEach((c) -> classProviders.put(c, pluginHandlerProvider)); + } + + // Iterate over the plugins, find loaded plugin instances for what the handler providers need + pluginsService.forEach((plugin) -> { + ImmutableClusterStateHandlerProvider handlerProvider = classProviders.get(plugin.getClass()); + if (handlerProvider != null) { + assert plugin.getClass().getClassLoader().equals(handlerProvider.getClass().getClassLoader()); + loadedPlugins.computeIfAbsent(handlerProvider, k -> new ArrayList<>()).add(plugin); + } + }); + + // Once we have the loaded plugins for each handler provider, get the handler instances and add them to the + // overall handler list + for (var providerPluginsEntry : loadedPlugins.entrySet()) { + handlers.addAll(providerPluginsEntry.getKey().handlers(providerPluginsEntry.getValue())); + } + + // Initialize the controller when merged + } + @Override protected void configure() { bind(ActionFilters.class).toInstance(actionFilters); diff --git a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java index 555c278182949..bb4fd1105cd79 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java @@ -8,6 +8,8 @@ package org.elasticsearch.immutablestate; +import org.elasticsearch.plugins.Plugin; + import java.util.Collection; /** @@ -21,5 +23,7 @@ public interface ImmutableClusterStateHandlerProvider { * * @return a list of ${@link ImmutableClusterStateHandler}s */ - Collection> handlers(); + Collection> handlers(Collection loadedPlugins); + + Collection> supportedPlugins(); } diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 7f87c76001c39..6c304da9bd401 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -1055,6 +1055,8 @@ protected Node( logger.debug("initializing HTTP handlers ..."); actionModule.initRestHandlers(() -> clusterService.state().nodesIfRecovered()); + logger.debug("initializing operator handlers ..."); + actionModule.initImmutableClusterStateHandlers(pluginsService); logger.info("initialized"); success = true; diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java index f124b13fb3434..2f2e92b951fde 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java @@ -9,23 +9,30 @@ import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; +import org.elasticsearch.plugins.Plugin; +import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.List; /** * ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface */ public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider { - private static volatile Collection> handlers = Collections.emptyList(); - @Override - public Collection> handlers() { - return handlers; + public Collection> handlers(Collection loadedPlugins) { + List> result = new ArrayList<>(); + // this will be much nicer with switch expressions + for (var plugin : loadedPlugins) { + if (plugin instanceof IndexLifecycle indexLifecycle) { + result.addAll(indexLifecycle.immutableClusterStateHandlers()); + } + } + return result; } - public static void registerHandlers(ImmutableClusterStateHandler... stateHandlers) { - handlers = List.of(stateHandlers); + @Override + public Collection> supportedPlugins() { + return List.of(IndexLifecycle.class); } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java index b7f824b4c2845..e477442bcc2e4 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java @@ -27,6 +27,7 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.health.HealthIndicatorService; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.license.XPackLicenseState; @@ -159,6 +160,7 @@ public class IndexLifecycle extends Plugin implements ActionPlugin, HealthPlugin private final SetOnce snapshotHistoryStore = new SetOnce<>(); private final SetOnce ilmHealthIndicatorService = new SetOnce<>(); private final SetOnce slmHealthIndicatorService = new SetOnce<>(); + private final SetOnce immutableLifecycleAction = new SetOnce<>(); private final Settings settings; public IndexLifecycle(Settings settings) { @@ -268,10 +270,8 @@ public Collection createComponents( components.addAll(Arrays.asList(snapshotLifecycleService.get(), snapshotHistoryStore.get(), snapshotRetentionService.get())); ilmHealthIndicatorService.set(new IlmHealthIndicatorService(clusterService)); slmHealthIndicatorService.set(new SlmHealthIndicatorService(clusterService)); + immutableLifecycleAction.set(new ImmutableLifecycleAction(xContentRegistry, client, XPackPlugin.getSharedLicenseState())); - ILMImmutableStateHandlerProvider.registerHandlers( - new ImmutableLifecycleAction(xContentRegistry, client, XPackPlugin.getSharedLicenseState()) - ); return components; } @@ -422,6 +422,10 @@ public List getRestHandlers( return actions; } + List> immutableClusterStateHandlers() { + return List.of(immutableLifecycleAction.get()); + } + @Override public Collection getHealthIndicatorServices() { return List.of(ilmHealthIndicatorService.get(), slmHealthIndicatorService.get()); From 12807d5357fd181ec2004643ccd879a6b62497c0 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 8 Jul 2022 14:29:39 -0400 Subject: [PATCH 3/6] Better naming --- .../java/org/elasticsearch/action/ActionModule.java | 12 ++++++------ .../ImmutableClusterStateHandlerProvider.java | 2 +- .../xpack/ilm/ILMImmutableStateHandlerProvider.java | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index b732a462cc236..bea6439951bcc 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -909,26 +909,26 @@ public void initImmutableClusterStateHandlers(PluginsService pluginsService) { // Add directly the handlers that the server has handlers.add(new ImmutableClusterSettingsAction(clusterSettings)); - Map, ImmutableClusterStateHandlerProvider> classProviders = new HashMap<>(); - Map> loadedPlugins = new HashMap<>(); + Map, ImmutableClusterStateHandlerProvider> classToProviderMap = new HashMap<>(); + Map> loadedPluginsForProvider = new HashMap<>(); // Get all plugin handler providers, map the plugin class they need to the provider for (var pluginHandlerProvider : pluginHandlerProviders) { - pluginHandlerProvider.supportedPlugins().forEach((c) -> classProviders.put(c, pluginHandlerProvider)); + pluginHandlerProvider.providedPlugins().forEach((c) -> classToProviderMap.put(c, pluginHandlerProvider)); } // Iterate over the plugins, find loaded plugin instances for what the handler providers need pluginsService.forEach((plugin) -> { - ImmutableClusterStateHandlerProvider handlerProvider = classProviders.get(plugin.getClass()); + ImmutableClusterStateHandlerProvider handlerProvider = classToProviderMap.get(plugin.getClass()); if (handlerProvider != null) { assert plugin.getClass().getClassLoader().equals(handlerProvider.getClass().getClassLoader()); - loadedPlugins.computeIfAbsent(handlerProvider, k -> new ArrayList<>()).add(plugin); + loadedPluginsForProvider.computeIfAbsent(handlerProvider, k -> new ArrayList<>()).add(plugin); } }); // Once we have the loaded plugins for each handler provider, get the handler instances and add them to the // overall handler list - for (var providerPluginsEntry : loadedPlugins.entrySet()) { + for (var providerPluginsEntry : loadedPluginsForProvider.entrySet()) { handlers.addAll(providerPluginsEntry.getKey().handlers(providerPluginsEntry.getValue())); } diff --git a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java index bb4fd1105cd79..db14afa4cea68 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java @@ -25,5 +25,5 @@ public interface ImmutableClusterStateHandlerProvider { */ Collection> handlers(Collection loadedPlugins); - Collection> supportedPlugins(); + Collection> providedPlugins(); } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java index 2f2e92b951fde..a379b925bfb9a 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java @@ -32,7 +32,7 @@ public Collection> handlers(Collection> supportedPlugins() { + public Collection> providedPlugins() { return List.of(IndexLifecycle.class); } } From 28f3828eaffc9f97ce9f68c88a1a6d595ef6ee2d Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 8 Jul 2022 15:01:59 -0400 Subject: [PATCH 4/6] Simplify with 1:1 mapping --- .../elasticsearch/action/ActionModule.java | 12 +++++------ .../ImmutableClusterStateHandlerProvider.java | 9 ++++++-- .../ilm/ILMImmutableStateHandlerProvider.java | 21 ++++++++----------- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index bea6439951bcc..9cc7a8856ee4e 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -910,25 +910,25 @@ public void initImmutableClusterStateHandlers(PluginsService pluginsService) { handlers.add(new ImmutableClusterSettingsAction(clusterSettings)); Map, ImmutableClusterStateHandlerProvider> classToProviderMap = new HashMap<>(); - Map> loadedPluginsForProvider = new HashMap<>(); + Map loadedPluginForProvider = new HashMap<>(); // Get all plugin handler providers, map the plugin class they need to the provider for (var pluginHandlerProvider : pluginHandlerProviders) { - pluginHandlerProvider.providedPlugins().forEach((c) -> classToProviderMap.put(c, pluginHandlerProvider)); + classToProviderMap.put(pluginHandlerProvider.parentPlugin(), pluginHandlerProvider); } - // Iterate over the plugins, find loaded plugin instances for what the handler providers need + // Iterate over the plugins, find loaded plugin instances for what the handler provider needs pluginsService.forEach((plugin) -> { ImmutableClusterStateHandlerProvider handlerProvider = classToProviderMap.get(plugin.getClass()); if (handlerProvider != null) { assert plugin.getClass().getClassLoader().equals(handlerProvider.getClass().getClassLoader()); - loadedPluginsForProvider.computeIfAbsent(handlerProvider, k -> new ArrayList<>()).add(plugin); + loadedPluginForProvider.put(handlerProvider, plugin); } }); - // Once we have the loaded plugins for each handler provider, get the handler instances and add them to the + // Once we have the loaded plugin for each handler provider, get the handler instances and add them to the // overall handler list - for (var providerPluginsEntry : loadedPluginsForProvider.entrySet()) { + for (var providerPluginsEntry : loadedPluginForProvider.entrySet()) { handlers.addAll(providerPluginsEntry.getKey().handlers(providerPluginsEntry.getValue())); } diff --git a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java index db14afa4cea68..44c79baa24921 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java @@ -23,7 +23,12 @@ public interface ImmutableClusterStateHandlerProvider { * * @return a list of ${@link ImmutableClusterStateHandler}s */ - Collection> handlers(Collection loadedPlugins); + Collection> handlers(Plugin loadedPlugin); - Collection> providedPlugins(); + /** + * Returns the class of the plugin which this service provider will use to return the handler list + * + * @return a class of the plugin related to the service provider + */ + Class parentPlugin(); } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java index a379b925bfb9a..c5b3fafa8b08f 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java @@ -11,28 +11,25 @@ import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; import org.elasticsearch.plugins.Plugin; -import java.util.ArrayList; import java.util.Collection; -import java.util.List; +import java.util.Collections; /** * ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface */ public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider { @Override - public Collection> handlers(Collection loadedPlugins) { - List> result = new ArrayList<>(); - // this will be much nicer with switch expressions - for (var plugin : loadedPlugins) { - if (plugin instanceof IndexLifecycle indexLifecycle) { - result.addAll(indexLifecycle.immutableClusterStateHandlers()); - } + public Collection> handlers(Plugin loadedPlugin) { + assert loadedPlugin instanceof IndexLifecycle; + if (loadedPlugin instanceof IndexLifecycle indexLifecycle) { + return indexLifecycle.immutableClusterStateHandlers(); } - return result; + + return Collections.emptyList(); } @Override - public Collection> providedPlugins() { - return List.of(IndexLifecycle.class); + public Class parentPlugin() { + return IndexLifecycle.class; } } From 11b4f95fe0e214e476986a1f2a151016e8c96585 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 11 Jul 2022 17:01:06 -0400 Subject: [PATCH 5/6] Use our createExtensions SPI --- .../elasticsearch/action/ActionModule.java | 30 ++----------------- .../ImmutableClusterStateHandlerProvider.java | 11 +------ .../elasticsearch/plugins/PluginsService.java | 13 ++++++-- .../plugins/PluginsServiceTests.java | 4 +++ .../ilm/ILMImmutableStateHandlerProvider.java | 20 ++++++------- 5 files changed, 26 insertions(+), 52 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index 9cc7a8856ee4e..0cae54985b1b8 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -276,7 +276,6 @@ import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.ActionPlugin.ActionHandler; -import org.elasticsearch.plugins.Plugin; import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.plugins.interceptor.RestInterceptorActionPlugin; import org.elasticsearch.rest.RestController; @@ -421,7 +420,6 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -902,36 +900,12 @@ public void initRestHandlers(Supplier nodesInCluster) { public void initImmutableClusterStateHandlers(PluginsService pluginsService) { List> handlers = new ArrayList<>(); - List pluginHandlerProviders = pluginsService.loadServiceProviders( + List pluginHandlers = pluginsService.loadServiceProviders( ImmutableClusterStateHandlerProvider.class ); - // Add directly the handlers that the server has handlers.add(new ImmutableClusterSettingsAction(clusterSettings)); - - Map, ImmutableClusterStateHandlerProvider> classToProviderMap = new HashMap<>(); - Map loadedPluginForProvider = new HashMap<>(); - - // Get all plugin handler providers, map the plugin class they need to the provider - for (var pluginHandlerProvider : pluginHandlerProviders) { - classToProviderMap.put(pluginHandlerProvider.parentPlugin(), pluginHandlerProvider); - } - - // Iterate over the plugins, find loaded plugin instances for what the handler provider needs - pluginsService.forEach((plugin) -> { - ImmutableClusterStateHandlerProvider handlerProvider = classToProviderMap.get(plugin.getClass()); - if (handlerProvider != null) { - assert plugin.getClass().getClassLoader().equals(handlerProvider.getClass().getClassLoader()); - loadedPluginForProvider.put(handlerProvider, plugin); - } - }); - - // Once we have the loaded plugin for each handler provider, get the handler instances and add them to the - // overall handler list - for (var providerPluginsEntry : loadedPluginForProvider.entrySet()) { - handlers.addAll(providerPluginsEntry.getKey().handlers(providerPluginsEntry.getValue())); - } - + pluginHandlers.forEach(h -> handlers.addAll(h.handlers())); // Initialize the controller when merged } diff --git a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java index 44c79baa24921..555c278182949 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java @@ -8,8 +8,6 @@ package org.elasticsearch.immutablestate; -import org.elasticsearch.plugins.Plugin; - import java.util.Collection; /** @@ -23,12 +21,5 @@ public interface ImmutableClusterStateHandlerProvider { * * @return a list of ${@link ImmutableClusterStateHandler}s */ - Collection> handlers(Plugin loadedPlugin); - - /** - * Returns the class of the plugin which this service provider will use to return the handler list - * - * @return a class of the plugin related to the service provider - */ - Class parentPlugin(); + Collection> handlers(); } diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java index 058885de79f81..7e2e13d5343f5 100644 --- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -309,7 +309,7 @@ public List loadServiceProviders(Class service) { List result = new ArrayList<>(); for (LoadedPlugin pluginTuple : plugins()) { - ServiceLoader.load(service, pluginTuple.loader()).iterator().forEachRemaining(c -> result.add(c)); + result.addAll(createExtensions(service, pluginTuple.instance)); } return Collections.unmodifiableList(result); @@ -348,11 +348,18 @@ static T createExtension(Class extensionClass, Class extensi throw new IllegalStateException("no public " + extensionConstructorMessage(extensionClass, extensionPointType)); } - if (constructors.length > 1) { + Constructor constructor = constructors[0]; + // Using modules and SPI requires that we declare the default no-arg constructor apart from our custom + // one arg constructor with a plugin. + if (constructors.length == 2) { + // we prefer the one arg constructor in this case + if (constructors[1].getParameterCount() > 0) { + constructor = constructors[1]; + } + } else if (constructors.length > 1) { throw new IllegalStateException("no unique public " + extensionConstructorMessage(extensionClass, extensionPointType)); } - final Constructor constructor = constructors[0]; if (constructor.getParameterCount() > 1) { throw new IllegalStateException(extensionSignatureMessage(extensionClass, extensionPointType, plugin)); } diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index ad334a2d42f8a..ac0e967face4f 100644 --- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -531,6 +531,10 @@ public TestExtension() {} public TestExtension(TestPlugin plugin) { } + + public TestExtension(TestPlugin plugin, String anotherArg) { + + } } IllegalStateException e = expectThrows( IllegalStateException.class, diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java index c5b3fafa8b08f..fb69802fbf09f 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java @@ -9,27 +9,25 @@ import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; -import org.elasticsearch.plugins.Plugin; import java.util.Collection; -import java.util.Collections; /** * ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface */ public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider { - @Override - public Collection> handlers(Plugin loadedPlugin) { - assert loadedPlugin instanceof IndexLifecycle; - if (loadedPlugin instanceof IndexLifecycle indexLifecycle) { - return indexLifecycle.immutableClusterStateHandlers(); - } + private final IndexLifecycle plugin; + + public ILMImmutableStateHandlerProvider() { + throw new IllegalStateException("Provider must be constructed using PluginsService"); + } - return Collections.emptyList(); + public ILMImmutableStateHandlerProvider(IndexLifecycle plugin) { + this.plugin = plugin; } @Override - public Class parentPlugin() { - return IndexLifecycle.class; + public Collection> handlers() { + return plugin.immutableClusterStateHandlers(); } } From 260db95fd193f634490c5ae6c5c7246f143e851f Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 11 Jul 2022 19:45:09 -0400 Subject: [PATCH 6/6] Remove unused code --- .../elasticsearch/action/ActionModule.java | 21 ------------------- .../java/org/elasticsearch/node/Node.java | 2 -- 2 files changed, 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index 0cae54985b1b8..1858d8ab28784 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -262,9 +262,6 @@ import org.elasticsearch.gateway.TransportNodesListGatewayStartedShards; import org.elasticsearch.health.GetHealthAction; import org.elasticsearch.health.RestGetHealthAction; -import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; -import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; -import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.index.seqno.GlobalCheckpointSyncAction; import org.elasticsearch.index.seqno.RetentionLeaseActions; import org.elasticsearch.indices.SystemIndices; @@ -276,7 +273,6 @@ import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.ActionPlugin.ActionHandler; -import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.plugins.interceptor.RestInterceptorActionPlugin; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; @@ -892,23 +888,6 @@ public void initRestHandlers(Supplier nodesInCluster) { registerHandler.accept(new RestCatAction(catActions)); } - /** - * Initializes the immutable cluster state handlers for Elasticsearch and it's modules/plugins - * - * @param pluginsService needed to load all modules/plugins immutable state handlers through SPI - */ - public void initImmutableClusterStateHandlers(PluginsService pluginsService) { - List> handlers = new ArrayList<>(); - - List pluginHandlers = pluginsService.loadServiceProviders( - ImmutableClusterStateHandlerProvider.class - ); - - handlers.add(new ImmutableClusterSettingsAction(clusterSettings)); - pluginHandlers.forEach(h -> handlers.addAll(h.handlers())); - // Initialize the controller when merged - } - @Override protected void configure() { bind(ActionFilters.class).toInstance(actionFilters); diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 6c304da9bd401..7f87c76001c39 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -1055,8 +1055,6 @@ protected Node( logger.debug("initializing HTTP handlers ..."); actionModule.initRestHandlers(() -> clusterService.state().nodesIfRecovered()); - logger.debug("initializing operator handlers ..."); - actionModule.initImmutableClusterStateHandlers(pluginsService); logger.info("initialized"); success = true;