Remove (again) test uses of onModule (#21414)

This change was reverted after it caused random test failures. This was
due to a copy/paste error in the original PR which caused the mock
version of ClusterInfoService to be used whenever the mock *ZenPing* was
used, and the real ClusterInfoService to be used when MockZenPing was
not used.

Author: rjernst

core/src/main/java/org/elasticsearch/cluster/ClusterModule.java

@@ -86,9 +86,6 @@ public class ClusterModule extends AbstractModule {
     final Collection<AllocationDecider> allocationDeciders;
     final ShardsAllocator shardsAllocator;
 
-    // pkg private so tests can mock
-    Class<? extends ClusterInfoService> clusterInfoServiceImpl = InternalClusterInfoService.class;
-
     public ClusterModule(Settings settings, ClusterService clusterService, List<ClusterPlugin> clusterPlugins) {
         this.settings = settings;
         this.allocationDeciders = createAllocationDeciders(settings, clusterService.getClusterSettings(), clusterPlugins);
@@ -159,7 +156,6 @@ private static ShardsAllocator createShardsAllocator(Settings settings, ClusterS
 
     @Override
     protected void configure() {
-        bind(ClusterInfoService.class).to(clusterInfoServiceImpl).asEagerSingleton();
         bind(GatewayAllocator.class).asEagerSingleton();
         bind(AllocationService.class).asEagerSingleton();
         bind(ClusterService.class).toInstance(clusterService);

core/src/main/java/org/elasticsearch/cluster/InternalClusterInfoService.java

@@ -19,17 +19,21 @@
 
 package org.elasticsearch.cluster;
 
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.LatchedActionListener;
 import org.elasticsearch.action.admin.cluster.node.stats.NodeStats;
 import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsRequest;
 import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsResponse;
-import org.elasticsearch.action.admin.cluster.node.stats.TransportNodesStatsAction;
 import org.elasticsearch.action.admin.indices.stats.IndicesStatsRequest;
 import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse;
 import org.elasticsearch.action.admin.indices.stats.ShardStats;
-import org.elasticsearch.action.admin.indices.stats.TransportIndicesStatsAction;
+import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.cluster.block.ClusterBlockException;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.metadata.MetaData;
@@ -39,7 +43,6 @@
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.collect.ImmutableOpenMap;
 import org.elasticsearch.common.component.AbstractComponent;
-import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
@@ -50,11 +53,6 @@
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.transport.ReceiveTimeoutTransportException;
 
-import java.util.List;
-import java.util.concurrent.CopyOnWriteArrayList;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
-
 /**
  * InternalClusterInfoService provides the ClusterInfoService interface,
  * routinely updated on a timer. The timer can be dynamically changed by
@@ -84,29 +82,24 @@ public class InternalClusterInfoService extends AbstractComponent implements Clu
     private volatile boolean isMaster = false;
     private volatile boolean enabled;
     private volatile TimeValue fetchTimeout;
-    private final TransportNodesStatsAction transportNodesStatsAction;
-    private final TransportIndicesStatsAction transportIndicesStatsAction;
     private final ClusterService clusterService;
     private final ThreadPool threadPool;
+    private final NodeClient client;
     private final List<Listener> listeners = new CopyOnWriteArrayList<>();
 
-    @Inject
-    public InternalClusterInfoService(Settings settings, ClusterSettings clusterSettings,
-                                      TransportNodesStatsAction transportNodesStatsAction,
-                                      TransportIndicesStatsAction transportIndicesStatsAction, ClusterService clusterService,
-                                      ThreadPool threadPool) {
+    public InternalClusterInfoService(Settings settings, ClusterService clusterService, ThreadPool threadPool, NodeClient client) {
         super(settings);
         this.leastAvailableSpaceUsages = ImmutableOpenMap.of();
         this.mostAvailableSpaceUsages = ImmutableOpenMap.of();
         this.shardRoutingToDataPath = ImmutableOpenMap.of();
         this.shardSizes = ImmutableOpenMap.of();
-        this.transportNodesStatsAction = transportNodesStatsAction;
-        this.transportIndicesStatsAction = transportIndicesStatsAction;
         this.clusterService = clusterService;
         this.threadPool = threadPool;
+        this.client = client;
         this.updateFrequency = INTERNAL_CLUSTER_INFO_UPDATE_INTERVAL_SETTING.get(settings);
         this.fetchTimeout = INTERNAL_CLUSTER_INFO_TIMEOUT_SETTING.get(settings);
         this.enabled = DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_THRESHOLD_ENABLED_SETTING.get(settings);
+        ClusterSettings clusterSettings = clusterService.getClusterSettings();
         clusterSettings.addSettingsUpdateConsumer(INTERNAL_CLUSTER_INFO_TIMEOUT_SETTING, this::setFetchTimeout);
         clusterSettings.addSettingsUpdateConsumer(INTERNAL_CLUSTER_INFO_UPDATE_INTERVAL_SETTING, this::setUpdateFrequency);
         clusterSettings.addSettingsUpdateConsumer(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_THRESHOLD_ENABLED_SETTING, this::setEnabled);
@@ -259,8 +252,7 @@ protected CountDownLatch updateNodeStats(final ActionListener<NodesStatsResponse
         nodesStatsRequest.clear();
         nodesStatsRequest.fs(true);
         nodesStatsRequest.timeout(fetchTimeout);
-
-        transportNodesStatsAction.execute(nodesStatsRequest, new LatchedActionListener<>(listener, latch));
+        client.admin().cluster().nodesStats(nodesStatsRequest, new LatchedActionListener<>(listener, latch));
         return latch;
     }
 
@@ -274,7 +266,7 @@ protected CountDownLatch updateIndicesStats(final ActionListener<IndicesStatsRes
         indicesStatsRequest.clear();
         indicesStatsRequest.store(true);
 
-        transportIndicesStatsAction.execute(indicesStatsRequest, new LatchedActionListener<>(listener, latch));
+        client.admin().indices().stats(indicesStatsRequest, new LatchedActionListener<>(listener, latch));
         return latch;
     }
 

core/src/main/java/org/elasticsearch/node/Node.java

@@ -35,9 +35,11 @@
 import org.elasticsearch.action.update.UpdateHelper;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.client.node.NodeClient;
+import org.elasticsearch.cluster.ClusterInfoService;
 import org.elasticsearch.cluster.ClusterModule;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.ClusterStateObserver;
+import org.elasticsearch.cluster.InternalClusterInfoService;
 import org.elasticsearch.cluster.MasterNodeChangePredicate;
 import org.elasticsearch.cluster.NodeConnectionsService;
 import org.elasticsearch.cluster.action.index.MappingUpdatedAction;
@@ -322,6 +324,7 @@ protected Node(final Environment environment, Collection<Class<? extends Plugin>
             for (final ExecutorBuilder<?> builder : threadPool.builders()) {
                 additionalSettings.addAll(builder.getRegisteredSettings());
             }
+            client = new NodeClient(settings, threadPool);
             final ResourceWatcherService resourceWatcherService = new ResourceWatcherService(settings, threadPool);
             final ScriptModule scriptModule = ScriptModule.create(settings, this.environment, resourceWatcherService,
                 pluginsService.filterPlugins(ScriptPlugin.class));
@@ -342,6 +345,7 @@ protected Node(final Environment environment, Collection<Class<? extends Plugin>
             resourcesToClose.add(tribeService);
             final IngestService ingestService = new IngestService(settings, threadPool, this.environment,
                 scriptModule.getScriptService(), analysisModule.getAnalysisRegistry(), pluginsService.filterPlugins(IngestPlugin.class));
+            final ClusterInfoService clusterInfoService = newClusterInfoService(settings, clusterService, threadPool, client);
 
             ModulesBuilder modules = new ModulesBuilder();
             // plugin modules must be added here, before others or we can get crazy injection errors...
@@ -377,7 +381,6 @@ protected Node(final Environment environment, Collection<Class<? extends Plugin>
                 .flatMap(Function.identity()).collect(Collectors.toList());
             final NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(namedWriteables);
             final MetaStateService metaStateService = new MetaStateService(settings, nodeEnvironment);
-            client = new NodeClient(settings, threadPool);
             final IndicesService indicesService = new IndicesService(settings, pluginsService, nodeEnvironment,
                 settingsModule.getClusterSettings(), analysisModule.getAnalysisRegistry(), searchModule.getQueryParserRegistry(),
                 clusterModule.getIndexNameExpressionResolver(), indicesModule.getMapperRegistry(), namedWriteableRegistry,
@@ -447,6 +450,7 @@ protected Node(final Environment environment, Collection<Class<? extends Plugin>
                     b.bind(UpdateHelper.class).toInstance(new UpdateHelper(settings, scriptModule.getScriptService()));
                     b.bind(MetaDataIndexUpgradeService.class).toInstance(new MetaDataIndexUpgradeService(settings,
                         indicesModule.getMapperRegistry(), settingsModule.getIndexScopedSettings()));
+                    b.bind(ClusterInfoService.class).toInstance(clusterInfoService);
                     b.bind(Discovery.class).toInstance(discoveryModule.getDiscovery());
                     b.bind(ZenPing.class).toInstance(discoveryModule.getZenPing());
                     {
@@ -916,4 +920,10 @@ protected ZenPing newZenPing(Settings settings, ThreadPool threadPool, Transport
     protected Node newTribeClientNode(Settings settings, Collection<Class<? extends Plugin>> classpathPlugins) {
         return new Node(new Environment(settings), classpathPlugins);
     }
+
+    /** Constructs a ClusterInfoService which may be mocked for tests. */
+    protected ClusterInfoService newClusterInfoService(Settings settings, ClusterService clusterService,
+                                                       ThreadPool threadPool, NodeClient client) {
+        return new InternalClusterInfoService(settings, clusterService, threadPool, client);
+    }
 }

core/src/main/java/org/elasticsearch/plugins/Plugin.java

@@ -25,6 +25,7 @@
 
 import org.elasticsearch.action.ActionModule;
 import org.elasticsearch.client.Client;
+import org.elasticsearch.cluster.ClusterModule;
 import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.component.LifecycleComponent;
@@ -38,6 +39,7 @@
 import org.elasticsearch.discovery.DiscoveryModule;
 import org.elasticsearch.index.IndexModule;
 import org.elasticsearch.indices.analysis.AnalysisModule;
+import org.elasticsearch.repositories.RepositoriesModule;
 import org.elasticsearch.script.ScriptModule;
 import org.elasticsearch.script.ScriptService;
 import org.elasticsearch.search.SearchModule;
@@ -224,6 +226,24 @@ public final void onModule(SearchModule module) {}
     @Deprecated
     public final void onModule(NetworkModule module) {}
 
+    /**
+     * Old-style snapshot/restore extension point. {@code @Deprecated} and {@code final} to act as a signpost for plugin authors upgrading
+     * from 2.x.
+     *
+     * @deprecated implement {@link RepositoryPlugin} instead
+     */
+    @Deprecated
+    public final void onModule(RepositoriesModule module) {}
+
+    /**
+     * Old-style cluster extension point. {@code @Deprecated} and {@code final} to act as a signpost for plugin authors upgrading
+     * from 2.x.
+     *
+     * @deprecated implement {@link ClusterPlugin} instead
+     */
+    @Deprecated
+    public final void onModule(ClusterModule module) {}
+
     /**
      * Old-style discovery extension point. {@code @Deprecated} and {@code final} to act as a signpost for plugin authors upgrading
      * from 2.x.

core/src/test/java/org/elasticsearch/indices/memory/breaker/RandomExceptionCircuitBreakerIT.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.indices.memory.breaker;
 
 import org.apache.lucene.index.DirectoryReader;
+import org.apache.lucene.index.FilterDirectoryReader;
 import org.apache.lucene.index.LeafReader;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.admin.cluster.node.stats.NodeStats;
@@ -39,16 +40,20 @@
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.indices.fielddata.cache.IndicesFieldDataCache;
 import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.search.basic.SearchWithRandomExceptionsIT;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.engine.MockEngineSupport;
 import org.elasticsearch.test.engine.ThrowingLeafReaderWrapper;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Random;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAllSuccessful;
@@ -60,7 +65,14 @@
 public class RandomExceptionCircuitBreakerIT extends ESIntegTestCase {
     @Override
     protected Collection<Class<? extends Plugin>> nodePlugins() {
-        return Arrays.asList(RandomExceptionDirectoryReaderWrapper.TestPlugin.class, MockEngineFactoryPlugin.class);
+        return Arrays.asList(RandomExceptionDirectoryReaderWrapper.TestPlugin.class);
+    }
+
+    @Override
+    protected Collection<Class<? extends Plugin>> getMockPlugins() {
+        Set<Class<? extends Plugin>> mocks = new HashSet<>(super.getMockPlugins());
+        mocks.remove(MockEngineFactoryPlugin.class);
+        return mocks;
     }
 
     public void testBreakerWithRandomExceptions() throws IOException, InterruptedException, ExecutionException {
@@ -200,14 +212,19 @@ public static class RandomExceptionDirectoryReaderWrapper extends MockEngineSupp
             Setting.doubleSetting(EXCEPTION_TOP_LEVEL_RATIO_KEY, 0.1d, 0.0d, Property.IndexScope);
         public static final Setting<Double> EXCEPTION_LOW_LEVEL_RATIO_SETTING =
             Setting.doubleSetting(EXCEPTION_LOW_LEVEL_RATIO_KEY, 0.1d, 0.0d, Property.IndexScope);
-        public static class TestPlugin extends Plugin {
+        public static class TestPlugin extends MockEngineFactoryPlugin {
             @Override
             public List<Setting<?>> getSettings() {
-                return Arrays.asList(EXCEPTION_TOP_LEVEL_RATIO_SETTING, EXCEPTION_LOW_LEVEL_RATIO_SETTING);
+                List<Setting<?>> settings = new ArrayList<>();
+                settings.addAll(super.getSettings());
+                settings.add(EXCEPTION_TOP_LEVEL_RATIO_SETTING);
+                settings.add(EXCEPTION_LOW_LEVEL_RATIO_SETTING);
+                return settings;
             }
 
-            public void onModule(MockEngineFactoryPlugin.MockEngineReaderModule module) {
-                module.setReaderClass(RandomExceptionDirectoryReaderWrapper.class);
+            @Override
+            protected Class<? extends FilterDirectoryReader> getReaderWrapperClass() {
+                return RandomExceptionDirectoryReaderWrapper.class;
             }
         }
 

core/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsIT.java

@@ -44,10 +44,13 @@
 import org.elasticsearch.test.engine.ThrowingLeafReaderWrapper;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Random;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
@@ -56,7 +59,14 @@ public class SearchWithRandomExceptionsIT extends ESIntegTestCase {
 
     @Override
     protected Collection<Class<? extends Plugin>> nodePlugins() {
-        return Arrays.asList(RandomExceptionDirectoryReaderWrapper.TestPlugin.class, MockEngineFactoryPlugin.class);
+        return Arrays.asList(RandomExceptionDirectoryReaderWrapper.TestPlugin.class);
+    }
+
+    @Override
+    protected Collection<Class<? extends Plugin>> getMockPlugins() {
+        Set<Class<? extends Plugin>> mocks = new HashSet<>(super.getMockPlugins());
+        mocks.remove(MockEngineFactoryPlugin.class);
+        return mocks;
     }
 
     public void testRandomExceptions() throws IOException, InterruptedException, ExecutionException {
@@ -153,17 +163,22 @@ public void testRandomExceptions() throws IOException, InterruptedException, Exe
 
     public static class RandomExceptionDirectoryReaderWrapper extends MockEngineSupport.DirectoryReaderWrapper {
 
-        public static class TestPlugin extends Plugin {
+        public static class TestPlugin extends MockEngineFactoryPlugin {
             public static final Setting<Double> EXCEPTION_TOP_LEVEL_RATIO_SETTING =
                 Setting.doubleSetting(EXCEPTION_TOP_LEVEL_RATIO_KEY, 0.1d, 0.0d, Property.IndexScope);
             public static final Setting<Double> EXCEPTION_LOW_LEVEL_RATIO_SETTING =
                 Setting.doubleSetting(EXCEPTION_LOW_LEVEL_RATIO_KEY, 0.1d, 0.0d, Property.IndexScope);
             @Override
             public List<Setting<?>> getSettings() {
-                return Arrays.asList(EXCEPTION_TOP_LEVEL_RATIO_SETTING, EXCEPTION_LOW_LEVEL_RATIO_SETTING);
+                List<Setting<?>> settings = new ArrayList<>();
+                settings.addAll(super.getSettings());
+                settings.add(EXCEPTION_TOP_LEVEL_RATIO_SETTING);
+                settings.add(EXCEPTION_LOW_LEVEL_RATIO_SETTING);
+                return settings;
             }
-            public void onModule(MockEngineFactoryPlugin.MockEngineReaderModule module) {
-                module.setReaderClass(RandomExceptionDirectoryReaderWrapper.class);
+            @Override
+            protected Class<? extends FilterDirectoryReader> getReaderWrapperClass() {
+                return RandomExceptionDirectoryReaderWrapper.class;
             }
         }
 

docs/reference/modules/scripting/native.asciidoc

@@ -20,17 +20,11 @@ If you squashed the whole thing into one class it'd look like:
 
 [source,java]
 --------------------------------------------------
-public class MyNativeScriptPlugin extends Plugin {
-    @Override
-    public String name() {
-        return "my-native-script";
-    }
+public class MyNativeScriptPlugin extends Plugin implements ScriptPlugin {
+
     @Override
-    public String description() {
-        return "my native script that does something great";
-    }
-    public void onModule(ScriptModule scriptModule) {
-        scriptModule.registerScript("my_script", MyNativeScriptFactory.class);
+    public List<NativeScriptFactory> getNativeScripts() {
+        return Collections.singletonList(new MyNativeScriptFactory());
     }
 
     public static class MyNativeScriptFactory implements NativeScriptFactory {

plugins/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java

@@ -46,7 +46,7 @@ public class IngestGeoIpPlugin extends Plugin implements IngestPlugin, Closeable
     @Override
     public Map<String, Processor.Factory> getProcessors(Processor.Parameters parameters) {
         if (databaseReaders != null) {
-            throw new IllegalStateException("called onModule twice for geoip plugin!!");
+            throw new IllegalStateException("getProcessors called twice for geoip plugin!!");
         }
         Path geoIpConfigDirectory = parameters.env.configFile().resolve("ingest-geoip");
         try {

plugins/jvm-example/src/main/java/org/elasticsearch/plugin/example/JvmExamplePlugin.java

@@ -60,9 +60,6 @@ public Settings additionalSettings() {
         return Settings.EMPTY;
     }
 
-    public void onModule(RepositoriesModule repositoriesModule) {
-    }
-
     /**
      * Module declaring some example configuration and a _cat action that uses
      * it.