Remove node filters for voting config exclusions

docs/reference/migration/migrate_8_0.asciidoc

@@ -16,6 +16,7 @@ coming[8.0.0]
 * <<breaking_80_allocation_changes>>
 * <<breaking_80_breaker_changes>>
 * <<breaking_80_discovery_changes>>
+* <<breaking_80_cluster_changes>>
 * <<breaking_80_mappings_changes>>
 * <<breaking_80_packaging_changes>>
 * <<breaking_80_rollup_changes>>
@@ -69,6 +70,7 @@ include::migrate_8_0/analysis.asciidoc[]
 include::migrate_8_0/allocation.asciidoc[]
 include::migrate_8_0/breaker.asciidoc[]
 include::migrate_8_0/discovery.asciidoc[]
+include::migrate_8_0/cluster.asciidoc[]
 include::migrate_8_0/mappings.asciidoc[]
 include::migrate_8_0/packaging.asciidoc[]
 include::migrate_8_0/rollup.asciidoc[]

docs/reference/migration/migrate_8_0/cluster.asciidoc

@@ -0,0 +1,18 @@
+[float]
+[[breaking_80_cluster_changes]]
+=== Cluster changes
+
+//NOTE: The notable-breaking-changes tagged regions are re-used in the
+//Installation and Upgrade Guide
+
+//tag::notable-breaking-changes[]
+
+// end::notable-breaking-changes[]
+
+[float]
+==== Change to API to add voting configuration exclusions
+
+The `POST /_cluster/voting_config_exclusions/{node_filter}` API has been
+removed in favour of `POST /_cluster/voting_config_exclusions?node_names=...`
+and `POST /_cluster/voting_config_exclusions?node_ids=...` which allow you to
+specify the names or IDs of the nodes to exclude.

docs/reference/migration/migrate_8_0/discovery.asciidoc

@@ -13,7 +13,7 @@
 ==== Removal of old discovery settings
 
 All settings under the `discovery.zen` namespace, which existed only for BWC reasons in 7.x,
-will no longer be supported. In particular, this includes:
+are no longer supported. In particular, this includes:
 
 - `discovery.zen.no_master_block`
 - `discovery.zen.hosts_provider`
@@ -36,4 +36,4 @@ will no longer be supported. In particular, this includes:
 - `discovery.zen.send_leave_request`
 - `discovery.zen.master_election.wait_for_joins_timeout`
 - `discovery.zen.master_election.ignore_non_master_pings`
-- `discovery.zen.publish.max_pending_cluster_states`
+- `discovery.zen.publish.max_pending_cluster_states`

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

@@ -26,21 +26,22 @@
 import org.elasticsearch.client.Node;
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.Response;
-import org.elasticsearch.client.ResponseException;
 import org.elasticsearch.client.RestClient;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.node.DiscoveryNodes;
 import org.elasticsearch.cluster.routing.UnassignedInfo;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.http.HttpServerTransport;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.InternalTestCluster;
-import org.hamcrest.Matchers;
 
 import java.io.IOException;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import static org.hamcrest.core.Is.is;
 
@@ -66,15 +67,27 @@ public void testRollingRestartOfTwoNodeCluster() throws Exception {
                 .build());
         ensureGreen("test");
 
+        final DiscoveryNodes discoveryNodes = client().admin().cluster().prepareState().clear().setNodes(true).get().getState().nodes();
+        final Map<String,String> nodeIdsByName = new HashMap<>(discoveryNodes.getSize());
+        discoveryNodes.forEach(n -> nodeIdsByName.put(n.getName(), n.getId()));
+
         RestClient restClient = getRestClient();
 
         internalCluster().rollingRestart(new InternalTestCluster.RestartCallback() {
             @Override
             public void doAfterNodes(int n, Client client) throws IOException {
                 ensureGreen("test");
-                Response response =
-                    restClient.performRequest(new Request("POST", "/_cluster/voting_config_exclusions/" +
-                        internalCluster().getNodeNames()[n]));
+                final Request request = new Request("POST", "/_cluster/voting_config_exclusions");
+                final String nodeName = internalCluster().getNodeNames()[n];
+                if (randomBoolean()) {
+                    request.addParameter("node_names", nodeName);
+                } else {
+                    final String nodeId = nodeIdsByName.get(nodeName);
+                    assertNotNull(nodeName, nodeId);
+                    request.addParameter("node_ids", nodeId);
+                }
+
+                final Response response = restClient.performRequest(request);
                 assertThat(response.getStatusLine().getStatusCode(), is(200));
             }
 
@@ -120,7 +133,9 @@ public void testClearVotingTombstonesNotWaitingForRemoval() throws Exception {
         List<String> nodes = internalCluster().startNodes(3);
         ensureStableCluster(3);
         RestClient restClient = getRestClient();
-        Response response = restClient.performRequest(new Request("POST", "/_cluster/voting_config_exclusions/" + nodes.get(2)));
+        final Request request = new Request("POST", "/_cluster/voting_config_exclusions");
+        request.addParameter("node_names", nodes.get(2));
+        final Response response = restClient.performRequest(request);
         assertThat(response.getStatusLine().getStatusCode(), is(200));
         assertThat(response.getEntity().getContentLength(), is(0L));
         Response deleteResponse = restClient.performRequest(
@@ -135,7 +150,9 @@ public void testClearVotingTombstonesWaitingForRemoval() throws Exception {
         ensureStableCluster(3);
         RestClient restClient = getRestClient();
         String nodeToWithdraw = nodes.get(randomIntBetween(0, 2));
-        Response response = restClient.performRequest(new Request("POST", "/_cluster/voting_config_exclusions/" + nodeToWithdraw));
+        final Request request = new Request("POST", "/_cluster/voting_config_exclusions");
+        request.addParameter("node_names", nodeToWithdraw);
+        final Response response = restClient.performRequest(request);
         assertThat(response.getStatusLine().getStatusCode(), is(200));
         assertThat(response.getEntity().getContentLength(), is(0L));
         internalCluster().stopRandomNode(InternalTestCluster.nameFilter(nodeToWithdraw));
@@ -144,30 +161,14 @@ public void testClearVotingTombstonesWaitingForRemoval() throws Exception {
         assertThat(deleteResponse.getEntity().getContentLength(), is(0L));
     }
 
-    public void testFailsOnUnknownNode() throws Exception {
-        internalCluster().setBootstrapMasterNodeIndex(2);
-        internalCluster().startNodes(3);
-        ensureStableCluster(3);
-        RestClient restClient = getRestClient();
-        try {
-            restClient.performRequest(new Request("POST", "/_cluster/voting_config_exclusions/invalid"));
-            fail("Invalid node name should throw.");
-        } catch (ResponseException e) {
-            assertThat(e.getResponse().getStatusLine().getStatusCode(), is(400));
-            assertThat(
-                e.getMessage(),
-                Matchers.containsString("add voting config exclusions request for [invalid] matched no master-eligible nodes")
-            );
-        }
-    }
-
     public void testRemoveTwoNodesAtOnce() throws Exception {
         internalCluster().setBootstrapMasterNodeIndex(2);
         List<String> nodes = internalCluster().startNodes(3);
         ensureStableCluster(3);
         RestClient restClient = getRestClient();
-        Response response = restClient.performRequest(new Request("POST", "/_cluster/voting_config_exclusions/" +
-            nodes.get(2) + "," + nodes.get(0)));
+        final Request request = new Request("POST", "/_cluster/voting_config_exclusions");
+        request.addParameter("node_names", nodes.get(2) + "," + nodes.get(0));
+        final Response response = restClient.performRequest(request);
         assertThat(response.getStatusLine().getStatusCode(), is(200));
         assertThat(response.getEntity().getContentLength(), is(0L));
         internalCluster().stopRandomNode(InternalTestCluster.nameFilter(nodes.get(0)));

server/src/main/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java

@@ -18,7 +18,7 @@
  */
 package org.elasticsearch.action.admin.cluster.configuration;
 
-import org.apache.logging.log4j.LogManager;
+import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.master.MasterNodeRequest;
 import org.elasticsearch.cluster.ClusterState;
@@ -28,7 +28,6 @@
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.unit.TimeValue;
 
 import java.io.IOException;
@@ -45,80 +44,63 @@
  * configuration.
  */
 public class AddVotingConfigExclusionsRequest extends MasterNodeRequest<AddVotingConfigExclusionsRequest> {
-    public static final String DEPRECATION_MESSAGE = "nodeDescription is deprecated and will be removed, use nodeIds or nodeNames instead";
-    private static final DeprecationLogger deprecationLogger = new DeprecationLogger(
-                                                                        LogManager.getLogger(AddVotingConfigExclusionsRequest.class));
-    private final String[] nodeDescriptions;
     private final String[] nodeIds;
     private final String[] nodeNames;
     private final TimeValue timeout;
 
     /**
      * Construct a request to add voting config exclusions for master-eligible nodes matching the given node names, and wait for a
      * default 30 seconds for these exclusions to take effect, removing the nodes from the voting configuration.
+     *
      * @param nodeNames Names of the nodes to add - see {@link AddVotingConfigExclusionsRequest#resolveVotingConfigExclusions(ClusterState)}
      */
     public AddVotingConfigExclusionsRequest(String... nodeNames) {
-        this(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, nodeNames, TimeValue.timeValueSeconds(30));
+        this(Strings.EMPTY_ARRAY, nodeNames, TimeValue.timeValueSeconds(30));
     }
 
     /**
      * Construct a request to add voting config exclusions for master-eligible nodes matching the given descriptions, and wait for these
      * nodes to be removed from the voting configuration.
-     * @param nodeDescriptions Descriptions of the nodes whose exclusions to add - see {@link DiscoveryNodes#resolveNodes(String...)}.
-     * @param nodeIds Ids of the nodes whose exclusions to add - see
+     *
+     * @param nodeIds   Ids of the nodes whose exclusions to add - see
      *                  {@link AddVotingConfigExclusionsRequest#resolveVotingConfigExclusions(ClusterState)}.
      * @param nodeNames Names of the nodes whose exclusions to add - see
      *                  {@link AddVotingConfigExclusionsRequest#resolveVotingConfigExclusions(ClusterState)}.
-     * @param timeout How long to wait for the added exclusions to take effect and be removed from the voting configuration.
+     * @param timeout   How long to wait for the added exclusions to take effect and be removed from the voting configuration.
      */
-    public AddVotingConfigExclusionsRequest(String[] nodeDescriptions, String[] nodeIds, String[] nodeNames, TimeValue timeout) {
+    public AddVotingConfigExclusionsRequest(String[] nodeIds, String[] nodeNames, TimeValue timeout) {
         if (timeout.compareTo(TimeValue.ZERO) < 0) {
             throw new IllegalArgumentException("timeout [" + timeout + "] must be non-negative");
         }
 
-        if (noneOrMoreThanOneIsSet(nodeDescriptions, nodeIds, nodeNames)) {
-            throw new IllegalArgumentException("Please set node identifiers correctly. " +
-                "One and only one of [node_name], [node_names] and [node_ids] has to be set");
+        if ((nodeIds.length > 0) == (nodeNames.length > 0)) {
+            throw new IllegalArgumentException("You must set [node_names] or [node_ids] but not both");
         }
 
-        if (nodeDescriptions.length > 0) {
-            deprecationLogger.deprecatedAndMaybeLog("voting_config_exclusion", DEPRECATION_MESSAGE);
-        }
-
-        this.nodeDescriptions = nodeDescriptions;
         this.nodeIds = nodeIds;
         this.nodeNames = nodeNames;
         this.timeout = timeout;
     }
 
     public AddVotingConfigExclusionsRequest(StreamInput in) throws IOException {
         super(in);
-        nodeDescriptions = in.readStringArray();
+        if (in.getVersion().before(Version.V_8_0_0)) {
+            final String[] legacyNodeDescriptions = in.readStringArray();
+            if (legacyNodeDescriptions.length > 0) {
+                throw new IllegalArgumentException("legacy [node_name] field was deprecated and must be empty");
+            }
+        }
         nodeIds = in.readStringArray();
         nodeNames = in.readStringArray();
         timeout = in.readTimeValue();
-
-        if (nodeDescriptions.length > 0) {
-            deprecationLogger.deprecatedAndMaybeLog("voting_config_exclusion",
-                "nodeDescription is deprecated and will be removed, use nodeIds or nodeNames instead");
-        }
-
+        assert (nodeIds.length > 0) != (nodeNames.length > 0);
     }
 
     Set<VotingConfigExclusion> resolveVotingConfigExclusions(ClusterState currentState) {
         final DiscoveryNodes allNodes = currentState.nodes();
         Set<VotingConfigExclusion> newVotingConfigExclusions = new HashSet<>();
 
-        if (nodeDescriptions.length >= 1) {
-            newVotingConfigExclusions = Arrays.stream(allNodes.resolveNodes(nodeDescriptions)).map(allNodes::get)
-                .filter(DiscoveryNode::isMasterNode).map(VotingConfigExclusion::new).collect(Collectors.toSet());
-
-            if (newVotingConfigExclusions.isEmpty()) {
-                throw new IllegalArgumentException("add voting config exclusions request for " + Arrays.asList(nodeDescriptions)
-                    + " matched no master-eligible nodes");
-            }
-        } else if (nodeIds.length >= 1) {
+        if (nodeIds.length > 0) {
             for (String nodeId : nodeIds) {
                 if (allNodes.nodeExists(nodeId)) {
                     DiscoveryNode discoveryNode = allNodes.get(nodeId);
@@ -130,12 +112,12 @@ Set<VotingConfigExclusion> resolveVotingConfigExclusions(ClusterState currentSta
                 }
             }
         } else {
-            assert nodeNames.length >= 1;
+            assert nodeNames.length > 0;
             Map<String, DiscoveryNode> existingNodes = StreamSupport.stream(allNodes.spliterator(), false)
-                                                                .collect(Collectors.toMap(DiscoveryNode::getName, Function.identity()));
+                .collect(Collectors.toMap(DiscoveryNode::getName, Function.identity()));
 
             for (String nodeName : nodeNames) {
-                if (existingNodes.containsKey(nodeName)){
+                if (existingNodes.containsKey(nodeName)) {
                     DiscoveryNode discoveryNode = existingNodes.get(nodeName);
                     if (discoveryNode.isMasterNode()) {
                         newVotingConfigExclusions.add(new VotingConfigExclusion(discoveryNode));
@@ -157,31 +139,17 @@ Set<VotingConfigExclusion> resolveVotingConfigExclusionsAndCheckMaximum(ClusterS
         final int oldExclusionsCount = currentState.getVotingConfigExclusions().size();
         final int newExclusionsCount = resolvedExclusions.size();
         if (oldExclusionsCount + newExclusionsCount > maxExclusionsCount) {
-            throw new IllegalArgumentException("add voting config exclusions request for " + Arrays.asList(nodeDescriptions)
+            throw new IllegalArgumentException("add voting config exclusions request for "
+                + (nodeNames.length > 0
+                    ? "nodes named " + Arrays.asList(nodeNames)
+                    : "nodes with ids " + Arrays.asList(nodeIds))
                 + " would add [" + newExclusionsCount + "] exclusions to the existing [" + oldExclusionsCount
                 + "] which would exceed the maximum of [" + maxExclusionsCount + "] set by ["
                 + maximumSettingKey + "]");
         }
         return resolvedExclusions;
     }
 
-    private boolean noneOrMoreThanOneIsSet(String[] deprecatedNodeDescription, String[] nodeIds, String[] nodeNames) {
-        if (deprecatedNodeDescription.length > 0) {
-            return nodeIds.length > 0 || nodeNames.length > 0;
-        } else if (nodeIds.length > 0) {
-            return nodeNames.length > 0;
-        } else {
-            return nodeNames.length > 0 == false;
-        }
-    }
-
-    /**
-     * @return descriptions of the nodes for whom to add voting config exclusions.
-     */
-    public String[] getNodeDescriptions() {
-        return nodeDescriptions;
-    }
-
     /**
      * @return ids of the nodes for whom to add voting config exclusions.
      */
@@ -211,7 +179,9 @@ public ActionRequestValidationException validate() {
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         super.writeTo(out);
-        out.writeStringArray(nodeDescriptions);
+        if (out.getVersion().before(Version.V_8_0_0)) {
+            out.writeStringArray(Strings.EMPTY_ARRAY);
+        }
         out.writeStringArray(nodeIds);
         out.writeStringArray(nodeNames);
         out.writeTimeValue(timeout);
@@ -220,7 +190,6 @@ public void writeTo(StreamOutput out) throws IOException {
     @Override
     public String toString() {
         return "AddVotingConfigExclusionsRequest{" +
-            "nodeDescriptions=" + Arrays.asList(nodeDescriptions) + ", " +
             "nodeIds=" + Arrays.asList(nodeIds) + ", " +
             "nodeNames=" + Arrays.asList(nodeNames) + ", " +
             "timeout=" + timeout +