Remove DiscoveryPlugin#getDiscoveryTypes (#38414)

With this change we no longer support pluggable discovery implementations. No
known implementations of `DiscoveryPlugin` actually override this method, so in
practice this should have no effect on the wider world. However, we were using
this rather extensively in tests to provide the `test-zen` discovery type. We
no longer need a separate discovery type for tests as we no longer need to
customise its behaviour.

Relates #38410

docs/reference/migration/migrate_7_0/plugins.asciidoc

@@ -70,3 +70,8 @@ The `RealmSettings.simpleString` method can be used as a convenience for the abo
 Tribe node functionality has been removed in favor of
 <<modules-cross-cluster-search,Cross Cluster Search>>.
 
+[float]
+==== Discovery implementations are no longer pluggable
+
+* The method `DiscoveryPlugin#getDiscoveryTypes()` was removed, so that plugins
+  can no longer provide their own discovery implementations.

modules/transport-netty4/src/test/java/org/elasticsearch/rest/discovery/Zen2RestApiIT.java

@@ -36,7 +36,6 @@
 import org.elasticsearch.http.HttpServerTransport;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.InternalTestCluster;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.hamcrest.Matchers;
 
 import java.io.IOException;
@@ -51,11 +50,6 @@
 @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0, transportClientRatio = 0, autoMinMasterNodes = false)
 public class Zen2RestApiIT extends ESNetty4IntegTestCase {
 
-    @Override
-    protected Settings nodeSettings(int nodeOrdinal) {
-        return Settings.builder().put(super.nodeSettings(nodeOrdinal)).put(TestZenDiscovery.USE_ZEN2.getKey(), true).build();
-    }
-
     @Override
     protected boolean addMockHttpTransport() {
         return false; // enable http

server/src/main/java/org/elasticsearch/discovery/DiscoveryModule.java

@@ -55,6 +55,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Random;
 import java.util.Set;
 import java.util.function.BiConsumer;
 import java.util.function.Function;
@@ -69,8 +70,8 @@
 public class DiscoveryModule {
     private static final Logger logger = LogManager.getLogger(DiscoveryModule.class);
 
-    public static final String ZEN_DISCOVERY_TYPE = "zen";
-    public static final String ZEN2_DISCOVERY_TYPE = "zen2";
+    public static final String ZEN_DISCOVERY_TYPE = "legacy-zen";
+    public static final String ZEN2_DISCOVERY_TYPE = "zen";
 
     public static final Setting<String> DISCOVERY_TYPE_SETTING =
         new Setting<>("discovery.type", ZEN2_DISCOVERY_TYPE, Function.identity(), Property.NodeScope);
@@ -136,17 +137,9 @@ public DiscoveryModule(Settings settings, ThreadPool threadPool, TransportServic
         discoveryTypes.put(ZEN2_DISCOVERY_TYPE, () -> new Coordinator(NODE_NAME_SETTING.get(settings), settings, clusterSettings,
             transportService, namedWriteableRegistry, allocationService, masterService,
             () -> gatewayMetaState.getPersistedState(settings, (ClusterApplierService) clusterApplier), hostsProvider, clusterApplier,
-            joinValidators, Randomness.get()));
+            joinValidators, new Random(Randomness.get().nextLong())));
         discoveryTypes.put("single-node", () -> new SingleNodeDiscovery(settings, transportService, masterService, clusterApplier,
             gatewayMetaState));
-        for (DiscoveryPlugin plugin : plugins) {
-            plugin.getDiscoveryTypes(threadPool, transportService, namedWriteableRegistry, masterService, clusterApplier, clusterSettings,
-                hostsProvider, allocationService, gatewayMetaState).forEach((key, value) -> {
-                if (discoveryTypes.put(key, value) != null) {
-                    throw new IllegalArgumentException("Cannot register discovery type [" + key + "] twice");
-                }
-            });
-        }
         String discoveryType = DISCOVERY_TYPE_SETTING.get(settings);
         Supplier<Discovery> discoverySupplier = discoveryTypes.get(discoveryType);
         if (discoverySupplier == null) {

server/src/main/java/org/elasticsearch/plugins/DiscoveryPlugin.java

@@ -19,26 +19,18 @@
 
 package org.elasticsearch.plugins;
 
-import java.util.Collections;
-import java.util.Map;
-import java.util.function.BiConsumer;
-import java.util.function.Supplier;
-
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.node.DiscoveryNode;
-import org.elasticsearch.cluster.routing.allocation.AllocationService;
-import org.elasticsearch.cluster.service.ClusterApplier;
-import org.elasticsearch.cluster.service.MasterService;
-import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.network.NetworkService;
-import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.discovery.Discovery;
 import org.elasticsearch.discovery.zen.UnicastHostsProvider;
-import org.elasticsearch.gateway.GatewayMetaState;
-import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.transport.TransportService;
 
+import java.util.Collections;
+import java.util.Map;
+import java.util.function.BiConsumer;
+import java.util.function.Supplier;
+
 /**
  * An additional extension point for {@link Plugin}s that extends Elasticsearch's discovery functionality. To add an additional
  * {@link NetworkService.CustomNameResolver} just implement the interface and implement the {@link #getCustomNameResolver(Settings)} method:
@@ -53,32 +45,6 @@
  * }</pre>
  */
 public interface DiscoveryPlugin {
-
-    /**
-     * Returns custom discovery implementations added by this plugin.
-     *
-     * The key of the returned map is the name of the discovery implementation
-     * (see {@link org.elasticsearch.discovery.DiscoveryModule#DISCOVERY_TYPE_SETTING}, and
-     * the value is a supplier to construct the {@link Discovery}.
-     *
-     * @param threadPool Use to schedule ping actions
-     * @param transportService Use to communicate with other nodes
-     * @param masterService Use to submit cluster state update tasks
-     * @param clusterApplier Use to locally apply cluster state updates
-     * @param clusterSettings Use to get cluster settings
-     * @param hostsProvider Use to find configured hosts which should be pinged for initial discovery
-     */
-    default Map<String, Supplier<Discovery>> getDiscoveryTypes(ThreadPool threadPool, TransportService transportService,
-                                                               NamedWriteableRegistry namedWriteableRegistry,
-                                                               MasterService masterService,
-                                                               ClusterApplier clusterApplier,
-                                                               ClusterSettings clusterSettings,
-                                                               UnicastHostsProvider hostsProvider,
-                                                               AllocationService allocationService,
-                                                               GatewayMetaState gatewayMetaState) {
-        return Collections.emptyMap();
-    }
-
     /**
      * Override to add additional {@link NetworkService.CustomNameResolver}s.
      * This can be handy if you want to provide your own Network interface name like _mycard_

server/src/test/java/org/elasticsearch/action/support/master/IndexingMasterFailoverIT.java

@@ -23,7 +23,6 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESIntegTestCase;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.elasticsearch.test.disruption.NetworkDisruption;
 import org.elasticsearch.test.disruption.NetworkDisruption.NetworkDisconnect;
 import org.elasticsearch.test.disruption.NetworkDisruption.TwoPartitions;
@@ -50,12 +49,6 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
         return classes;
     }
 
-    @Override
-    protected Settings nodeSettings(int nodeOrdinal) {
-        return Settings.builder().put(super.nodeSettings(nodeOrdinal))
-            .put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false).build();
-    }
-
     /**
      * Indexing operations which entail mapping changes require a blocking request to the master node to update the mapping.
      * If the master node is being disrupted or if it cannot commit cluster state changes, it needs to retry within timeout limits.

server/src/test/java/org/elasticsearch/client/transport/TransportClientIT.java

@@ -34,7 +34,6 @@
 import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
 import org.elasticsearch.test.ESIntegTestCase.Scope;
 import org.elasticsearch.test.MockHttpTransport;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.elasticsearch.transport.MockTransportClient;
 import org.elasticsearch.transport.TransportService;
 
@@ -66,10 +65,8 @@ public void testNodeVersionIsUpdated() throws IOException, NodeValidationExcepti
                 .put("transport.type", getTestTransportType())
                 .put(Node.NODE_DATA_SETTING.getKey(), false)
                 .put("cluster.name", "foobar")
-                .put(TestZenDiscovery.USE_ZEN2.getKey(), getUseZen2())
                 .putList(ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.getKey(), "testNodeVersionIsUpdated")
-                .build(), Arrays.asList(getTestTransportPlugin(), TestZenDiscovery.TestPlugin.class,
-                                        MockHttpTransport.TestPlugin.class)).start()) {
+                .build(), Arrays.asList(getTestTransportPlugin(), MockHttpTransport.TestPlugin.class)).start()) {
             TransportAddress transportAddress = node.injector().getInstance(TransportService.class).boundAddress().publishAddress();
             client.addTransportAddress(transportAddress);
             // since we force transport clients there has to be one node started that we connect to.

server/src/test/java/org/elasticsearch/cluster/coordination/RareClusterStateIT.java

@@ -49,7 +49,6 @@
 import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.test.ESIntegTestCase;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.elasticsearch.test.disruption.BlockClusterStateProcessing;
 import org.elasticsearch.test.junit.annotations.TestLogging;
 
@@ -71,12 +70,6 @@
 @TestLogging("_root:DEBUG")
 public class RareClusterStateIT extends ESIntegTestCase {
 
-    @Override
-    protected Settings nodeSettings(int nodeOrdinal) {
-        return Settings.builder().put(super.nodeSettings(nodeOrdinal))
-            .put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false).build();
-    }
-
     @Override
     protected int numberOfShards() {
         return 1;

server/src/test/java/org/elasticsearch/cluster/coordination/Zen1IT.java

@@ -37,13 +37,13 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.discovery.Discovery;
+import org.elasticsearch.discovery.DiscoveryModule;
 import org.elasticsearch.discovery.zen.ElectMasterService;
 import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.gateway.MetaStateService;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.InternalTestCluster.RestartCallback;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.elasticsearch.test.transport.MockTransportService;
 import org.elasticsearch.transport.TransportService;
 
@@ -71,12 +71,10 @@
 public class Zen1IT extends ESIntegTestCase {
 
     private static Settings ZEN1_SETTINGS = Coordinator.addZen1Attribute(true, Settings.builder()
-        .put(TestZenDiscovery.USE_ZEN2.getKey(), false)
-        .put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false)) // Zen2 does not know about mock pings
-        .build();
+        .put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), DiscoveryModule.ZEN_DISCOVERY_TYPE)).build();
 
     private static Settings ZEN2_SETTINGS = Settings.builder()
-        .put(TestZenDiscovery.USE_ZEN2.getKey(), true)
+        .put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), DiscoveryModule.ZEN2_DISCOVERY_TYPE)
         .build();
 
     protected Collection<Class<? extends Plugin>> nodePlugins() {

server/src/test/java/org/elasticsearch/cluster/routing/PrimaryAllocationIT.java

@@ -47,7 +47,6 @@
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.InternalTestCluster;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.elasticsearch.test.disruption.NetworkDisruption;
 import org.elasticsearch.test.disruption.NetworkDisruption.NetworkDisconnect;
 import org.elasticsearch.test.disruption.NetworkDisruption.TwoPartitions;
@@ -88,12 +87,6 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
         return Arrays.asList(MockTransportService.TestPlugin.class);
     }
 
-    @Override
-    protected Settings nodeSettings(int nodeOrdinal) {
-        return Settings.builder().put(super.nodeSettings(nodeOrdinal))
-            .put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false).build();
-    }
-
     public void testBulkWeirdScenario() throws Exception {
         String master = internalCluster().startMasterOnlyNode(Settings.EMPTY);
         internalCluster().startDataOnlyNodes(2);

server/src/test/java/org/elasticsearch/discovery/AbstractDisruptionTestCase.java

@@ -30,12 +30,9 @@
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.discovery.zen.UnicastZenPing;
-import org.elasticsearch.discovery.zen.ZenPing;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.InternalTestCluster;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.elasticsearch.test.disruption.NetworkDisruption;
 import org.elasticsearch.test.disruption.NetworkDisruption.Bridge;
 import org.elasticsearch.test.disruption.NetworkDisruption.DisruptedLinks;
@@ -65,8 +62,7 @@ public abstract class AbstractDisruptionTestCase extends ESIntegTestCase {
 
     @Override
     protected Settings nodeSettings(int nodeOrdinal) {
-        return Settings.builder().put(super.nodeSettings(nodeOrdinal)).put(DEFAULT_SETTINGS)
-                .put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false).build();
+        return Settings.builder().put(super.nodeSettings(nodeOrdinal)).put(DEFAULT_SETTINGS).build();
     }
 
     @Override
@@ -114,22 +110,9 @@ List<String> startCluster(int numberOfNodes) {
         InternalTestCluster internalCluster = internalCluster();
         List<String> nodes = internalCluster.startNodes(numberOfNodes);
         ensureStableCluster(numberOfNodes);
-
-        // TODO: this is a temporary solution so that nodes will not base their reaction to a partition based on previous successful results
-        clearTemporalResponses();
         return nodes;
     }
 
-    protected void clearTemporalResponses() {
-        final Discovery discovery = internalCluster().getInstance(Discovery.class);
-        if (discovery instanceof TestZenDiscovery) {
-            ZenPing zenPing = ((TestZenDiscovery) discovery).getZenPing();
-            if (zenPing instanceof UnicastZenPing) {
-                ((UnicastZenPing) zenPing).clearTemporalResponses();
-            }
-        }
-    }
-
     static final Settings DEFAULT_SETTINGS = Settings.builder()
             .put(LeaderChecker.LEADER_CHECK_TIMEOUT_SETTING.getKey(), "1s") // for hitting simulated network failures quickly
             .put(LeaderChecker.LEADER_CHECK_RETRY_COUNT_SETTING.getKey(), 1) // for hitting simulated network failures quickly

server/src/test/java/org/elasticsearch/discovery/DiscoveryModuleTests.java

@@ -22,7 +22,6 @@
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.coordination.Coordinator;
 import org.elasticsearch.cluster.node.DiscoveryNode;
-import org.elasticsearch.cluster.routing.allocation.AllocationService;
 import org.elasticsearch.cluster.service.ClusterApplier;
 import org.elasticsearch.cluster.service.MasterService;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
@@ -36,7 +35,6 @@
 import org.elasticsearch.gateway.GatewayMetaState;
 import org.elasticsearch.plugins.DiscoveryPlugin;
 import org.elasticsearch.test.ESTestCase;
-import org.elasticsearch.test.NoopDiscovery;
 import org.elasticsearch.test.transport.MockTransportService;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.transport.TransportService;
@@ -75,18 +73,6 @@ default Map<String, Supplier<UnicastHostsProvider>> getZenHostsProviders(Transpo
         }
     }
 
-    public interface DummyDiscoveryPlugin extends DiscoveryPlugin {
-        Map<String, Supplier<Discovery>> impl();
-        @Override
-        default Map<String, Supplier<Discovery>> getDiscoveryTypes(ThreadPool threadPool, TransportService transportService,
-                                                                   NamedWriteableRegistry namedWriteableRegistry,
-                                                                   MasterService masterService, ClusterApplier clusterApplier,
-                                                                   ClusterSettings clusterSettings, UnicastHostsProvider hostsProvider,
-                                                                   AllocationService allocationService, GatewayMetaState gatewayMetaState) {
-            return impl();
-        }
-    }
-
     @Before
     public void setupDummyServices() {
         threadPool = mock(ThreadPool.class);
@@ -114,34 +100,13 @@ public void testDefaults() {
         assertTrue(module.getDiscovery() instanceof Coordinator);
     }
 
-    public void testLazyConstructionDiscovery() {
-        DummyDiscoveryPlugin plugin = () -> Collections.singletonMap("custom",
-            () -> { throw new AssertionError("created discovery type which was not selected"); });
-        newModule(Settings.EMPTY, Collections.singletonList(plugin));
-    }
-
-    public void testRegisterDiscovery() {
-        Settings settings = Settings.builder().put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), "custom").build();
-        DummyDiscoveryPlugin plugin = () -> Collections.singletonMap("custom", NoopDiscovery::new);
-        DiscoveryModule module = newModule(settings, Collections.singletonList(plugin));
-        assertTrue(module.getDiscovery() instanceof NoopDiscovery);
-    }
-
     public void testUnknownDiscovery() {
         Settings settings = Settings.builder().put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), "dne").build();
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
             newModule(settings, Collections.emptyList()));
         assertEquals("Unknown discovery type [dne]", e.getMessage());
     }
 
-    public void testDuplicateDiscovery() {
-        DummyDiscoveryPlugin plugin1 = () -> Collections.singletonMap("dup", () -> null);
-        DummyDiscoveryPlugin plugin2 = () -> Collections.singletonMap("dup", () -> null);
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
-            newModule(Settings.EMPTY, Arrays.asList(plugin1, plugin2)));
-        assertEquals("Cannot register discovery type [dup] twice", e.getMessage());
-    }
-
     public void testHostsProvider() {
         Settings settings = Settings.builder().put(DiscoveryModule.DISCOVERY_SEED_PROVIDERS_SETTING.getKey(), "custom").build();
         AtomicBoolean created = new AtomicBoolean(false);

server/src/test/java/org/elasticsearch/discovery/SnapshotDisruptionIT.java

@@ -18,8 +18,6 @@
  */
 package org.elasticsearch.discovery;
 
-import java.util.Arrays;
-import java.util.Collection;
 import org.elasticsearch.action.ActionFuture;
 import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
 import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse;
@@ -38,17 +36,18 @@
 import org.elasticsearch.snapshots.SnapshotMissingException;
 import org.elasticsearch.snapshots.SnapshotState;
 import org.elasticsearch.test.ESIntegTestCase;
-import org.elasticsearch.test.discovery.TestZenDiscovery;
 import org.elasticsearch.test.disruption.NetworkDisruption;
 import org.elasticsearch.test.junit.annotations.TestLogging;
+import org.elasticsearch.test.transport.MockTransportService;
 
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
-import org.elasticsearch.test.transport.MockTransportService;
 
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 
@@ -68,7 +67,6 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
     protected Settings nodeSettings(int nodeOrdinal) {
         return Settings.builder().put(super.nodeSettings(nodeOrdinal))
             .put(AbstractDisruptionTestCase.DEFAULT_SETTINGS)
-            .put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false)
             .build();
     }