Remove node.client setting

As discussed in #16565, the node.client setting is an unnecessary shortcut to node.data: false and node.master: false. We have places where we treat nodes with node.client set to true differently compared to master false and data false, which is not correct. Also, with the addition of node.ingest or potentially new roles, it becomes confusing to figure out if a node client should support ingestion or not.

This commit removes the node.client setting in favour being explicit using node.master, node.data and node.ingest instead.

core/src/main/java/org/elasticsearch/action/support/ThreadedActionListener.java

@@ -22,7 +22,6 @@ package org.elasticsearch.action.support;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.client.transport.TransportClient;
-import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.AbstractRunnable;
@@ -48,9 +47,9 @@ public final class ThreadedActionListener<Response> implements ActionListener<Re
         public Wrapper(ESLogger logger, Settings settings, ThreadPool threadPool) {
             this.logger = logger;
             this.threadPool = threadPool;
-             // Should the action listener be threaded or not by default. Action listeners are automatically threaded for client
+             // Should the action listener be threaded or not by default. Action listeners are automatically threaded for
-             // nodes and transport client in order to make sure client side code is not executed on IO threads.
+            // the transport client in order to make sure client side code is not executed on IO threads.
-            this.threadedListener = DiscoveryNode.clientNode(settings) || TransportClient.CLIENT_TYPE.equals(Client.CLIENT_TYPE_SETTING_S.get(settings));
+            this.threadedListener = TransportClient.CLIENT_TYPE.equals(Client.CLIENT_TYPE_SETTING_S.get(settings));
         }
 
         public <Response> ActionListener<Response> wrap(ActionListener<Response> listener) {

core/src/main/java/org/elasticsearch/client/transport/TransportClient.java

@@ -112,7 +112,10 @@ public class TransportClient extends AbstractClient {
                     .put(NettyTransport.PING_SCHEDULE.getKey(), "5s") // enable by default the transport schedule ping interval
                     .put(InternalSettingsPreparer.prepareSettings(settings))
                     .put(NetworkService.NETWORK_SERVER.getKey(), false)
-                    .put(Node.NODE_CLIENT_SETTING.getKey(), true)
+                    //nocommit not too sure if these settings are needed here...
+                    .put(Node.NODE_MASTER_SETTING.getKey(), false)
+                    .put(Node.NODE_DATA_SETTING.getKey(), false)
+                    .put(Node.NODE_INGEST_SETTING.getKey(), false)
                     .put(CLIENT_TYPE_SETTING_S.getKey(), CLIENT_TYPE);
             return new PluginsService(settingsBuilder.build(), null, null, pluginClasses);
         }

core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java

@@ -48,7 +48,6 @@ public class DiscoveryNode implements Streamable, ToXContent {
 
     public static final String DATA_ATTR = "data";
     public static final String MASTER_ATTR = "master";
-    public static final String CLIENT_ATTR = "client";
     public static final String INGEST_ATTR = "ingest";
 
     public static boolean localNode(Settings settings) {
@@ -69,25 +68,15 @@ public class DiscoveryNode implements Streamable, ToXContent {
     }
 
     public static boolean nodeRequiresLocalStorage(Settings settings) {
-        return (Node.NODE_CLIENT_SETTING.get(settings) || (Node.NODE_DATA_SETTING.get(settings) == false && Node.NODE_MASTER_SETTING.get(settings) == false)) == false;
+        return Node.NODE_DATA_SETTING.get(settings) || Node.NODE_MASTER_SETTING.get(settings);
-    }
-
-    public static boolean clientNode(Settings settings) {
-        return Node.NODE_CLIENT_SETTING.get(settings);
     }
 
     public static boolean masterNode(Settings settings) {
-        if (Node.NODE_MASTER_SETTING.exists(settings)) {
+        return Node.NODE_MASTER_SETTING.get(settings);
-            return Node.NODE_MASTER_SETTING.get(settings);
-        }
-        return clientNode(settings) == false;
     }
 
     public static boolean dataNode(Settings settings) {
-        if (Node.NODE_DATA_SETTING.exists(settings)) {
+        return Node.NODE_DATA_SETTING.get(settings);
-            return Node.NODE_DATA_SETTING.get(settings);
-        }
-        return clientNode(settings) == false;
     }
 
     public static boolean ingestNode(Settings settings) {
@@ -270,10 +259,7 @@ public class DiscoveryNode implements Streamable, ToXContent {
      */
     public boolean dataNode() {
         String data = attributes.get(DATA_ATTR);
-        if (data == null) {
+        return data == null ? true : Booleans.parseBooleanExact(data);
-            return !clientNode();
-        }
-        return Booleans.parseBooleanExact(data);
     }
 
     /**
@@ -283,27 +269,12 @@ public class DiscoveryNode implements Streamable, ToXContent {
         return dataNode();
     }
 
-    /**
-     * Is the node a client node or not.
-     */
-    public boolean clientNode() {
-        String client = attributes.get(CLIENT_ATTR);
-        return client != null && Booleans.parseBooleanExact(client);
-    }
-
-    public boolean isClientNode() {
-        return clientNode();
-    }
-
     /**
      * Can this node become master or not.
      */
     public boolean masterNode() {
-        String master = attributes.get(MASTER_ATTR);
+            String master = attributes.get(MASTER_ATTR);
-        if (master == null) {
+        return master == null ? true : Booleans.parseBooleanExact(master);
-            return !clientNode();
-        }
-        return Booleans.parseBooleanExact(master);
     }
 
     /**

core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeService.java

@@ -22,6 +22,7 @@ package org.elasticsearch.cluster.node;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.node.Node;
 
 import java.util.HashMap;
 import java.util.List;
@@ -45,16 +46,13 @@ public class DiscoveryNodeService extends AbstractComponent {
     }
 
     public Map<String, String> buildAttributes() {
-        Map<String, String> attributes = new HashMap<>(settings.getByPrefix("node.").getAsMap());
+        Map<String, String> attributes = new HashMap<>(Node.NODE_ATTRIBUTES.get(this.settings).getAsMap());
         attributes.remove("name"); // name is extracted in other places
         if (attributes.containsKey("client")) {
-            if (attributes.get("client").equals("false")) {
+            throw new IllegalArgumentException("node.client setting is no longer supported, use " + Node.NODE_MASTER_SETTING.getKey()
-                attributes.remove("client"); // this is the default
+                + ", " + Node.NODE_DATA_SETTING.getKey() + " and " + Node.NODE_INGEST_SETTING.getKey() + " explicitly instead");
-            } else {
-                // if we are client node, don't store data ...
-                attributes.put("data", "false");
-            }
         }
+        //nocommit why don't we remove master as well if it's true? and ingest?
         if (attributes.containsKey("data")) {
             if (attributes.get("data").equals("true")) {
                 attributes.remove("data");
@@ -79,7 +77,7 @@ public class DiscoveryNodeService extends AbstractComponent {
         return attributes;
     }
 
-    public static interface CustomAttributesProvider {
+    public interface CustomAttributesProvider {
 
         Map<String, String> buildAttributes();
     }

core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

@@ -350,7 +350,6 @@ public final class ClusterSettings extends AbstractScopedSettings {
                     SearchService.KEEPALIVE_INTERVAL_SETTING,
                     Node.WRITE_PORTS_FIELD_SETTING,
                     Node.NODE_NAME_SETTING,
-                    Node.NODE_CLIENT_SETTING,
                     Node.NODE_DATA_SETTING,
                     Node.NODE_MASTER_SETTING,
                     Node.NODE_LOCAL_SETTING,

core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java

@@ -853,10 +853,11 @@ public class ZenDiscovery extends AbstractLifecycleComponent<Discovery> implemen
         List<ZenPing.PingResponse> pingResponses = new ArrayList<>();
         for (ZenPing.PingResponse pingResponse : fullPingResponses) {
             DiscoveryNode node = pingResponse.node();
-            if (masterElectionFilterClientNodes && (node.clientNode() || (!node.masterNode() && !node.dataNode()))) {
+            //nocommit we should rename this and its setting, also we ignore node.ingest, but maybe it's ok here
-                // filter out the client node, which is a client node, or also one that is not data and not master (effectively, client)
+            if (masterElectionFilterClientNodes && node.masterNode() == false && node.dataNode() == false) {
-            } else if (masterElectionFilterDataNodes && (!node.masterNode() && node.dataNode())) {
+                // filter out nodes that don't hold data and are not master eligible
-                // filter out data node that is not also master
+            } else if (masterElectionFilterDataNodes && node.masterNode() == false && node.dataNode()) {
+                // filter out dedicated data nodes
             } else {
                 pingResponses.add(pingResponse);
             }

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

@@ -130,14 +130,13 @@ import static org.elasticsearch.common.settings.Settings.settingsBuilder;
 public class Node implements Closeable {
 
     public static final Setting<Boolean> WRITE_PORTS_FIELD_SETTING = Setting.boolSetting("node.portsfile", false, false, Setting.Scope.CLUSTER);
-    public static final Setting<Boolean> NODE_CLIENT_SETTING = Setting.boolSetting("node.client", false, false, Setting.Scope.CLUSTER);
     public static final Setting<Boolean> NODE_DATA_SETTING = Setting.boolSetting("node.data", true, false, Setting.Scope.CLUSTER);
     public static final Setting<Boolean> NODE_MASTER_SETTING = Setting.boolSetting("node.master", true, false, Setting.Scope.CLUSTER);
     public static final Setting<Boolean> NODE_LOCAL_SETTING = Setting.boolSetting("node.local", false, false, Setting.Scope.CLUSTER);
     public static final Setting<String> NODE_MODE_SETTING = new Setting<>("node.mode", "network", Function.identity(), false, Setting.Scope.CLUSTER);
     public static final Setting<Boolean> NODE_INGEST_SETTING = Setting.boolSetting("node.ingest", true, false, Setting.Scope.CLUSTER);
     public static final Setting<String> NODE_NAME_SETTING = Setting.simpleString("node.name", false, Setting.Scope.CLUSTER);
-    // this sucks that folks can mistype client etc and get away with it.
+    // this sucks that folks can mistype data, master or ingest and get away with it.
     // TODO: we should move this to node.attribute.${name} = ${value} instead.
     public static final Setting<Settings> NODE_ATTRIBUTES = Setting.groupSetting("node.", false, Setting.Scope.CLUSTER);
 

core/src/main/java/org/elasticsearch/tribe/TribeService.java

@@ -107,7 +107,9 @@ public class TribeService extends AbstractLifecycleComponent<TribeService> {
         }
         // its a tribe configured node..., force settings
         Settings.Builder sb = Settings.builder().put(settings);
-        sb.put(Node.NODE_CLIENT_SETTING.getKey(), true); // this node should just act as a node client
+        sb.put(Node.NODE_MASTER_SETTING.getKey(), false);
+        sb.put(Node.NODE_DATA_SETTING.getKey(), false);
+        sb.put(Node.NODE_INGEST_SETTING.getKey(), false);
         sb.put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), "local"); // a tribe node should not use zen discovery
         // nothing is going to be discovered, since no master will be elected
         sb.put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), 0);
@@ -177,7 +179,9 @@ public class TribeService extends AbstractLifecycleComponent<TribeService> {
             if (sb.get("http.enabled") == null) {
                 sb.put("http.enabled", false);
             }
-            sb.put(Node.NODE_CLIENT_SETTING.getKey(), true);
+            sb.put(Node.NODE_DATA_SETTING.getKey(), false);
+            sb.put(Node.NODE_MASTER_SETTING.getKey(), false);
+            sb.put(Node.NODE_INGEST_SETTING.getKey(), false);
             nodes.add(new TribeClientNode(sb.build()));
         }
 

core/src/test/java/org/elasticsearch/action/ListenerActionIT.java

@@ -23,7 +23,6 @@ import org.elasticsearch.action.index.IndexRequest;
 import org.elasticsearch.action.index.IndexResponse;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.client.transport.TransportClient;
-import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.test.ESIntegTestCase;
 
 import java.util.concurrent.CountDownLatch;
@@ -61,7 +60,7 @@ public class ListenerActionIT extends ESIntegTestCase {
 
         latch.await();
 
-        boolean shouldBeThreaded = DiscoveryNode.clientNode(client.settings()) || TransportClient.CLIENT_TYPE.equals(Client.CLIENT_TYPE_SETTING_S.get(client.settings()));
+        boolean shouldBeThreaded = TransportClient.CLIENT_TYPE.equals(Client.CLIENT_TYPE_SETTING_S.get(client.settings()));
         if (shouldBeThreaded) {
             assertTrue(threadName.get().contains("listener"));
         } else {

core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java

@@ -0,0 +1,63 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.cluster.node;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.test.ESTestCase;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+
+public class DiscoveryNodeServiceTests extends ESTestCase {
+
+    public void testClientNodeSettingIsProhibited() {
+        Settings settings = Settings.builder().put("node.client", randomBoolean()).build();
+        try {
+            new DiscoveryNodeService(settings).buildAttributes();
+            fail("build attributes should have failed");
+        } catch(IllegalArgumentException e) {
+            assertThat(e.getMessage(), equalTo("node.client setting is no longer supported, use node.master, " +
+                    "node.data and node.ingest explicitly instead"));
+        }
+    }
+
+    public void testBuildAttributesWithCustomAttributeServiceProvider() {
+        Map<String, String> expectedAttributes = new HashMap<>();
+        int numCustomSettings = randomIntBetween(0, 5);
+        Settings.Builder builder = Settings.builder();
+        for (int i = 0; i < numCustomSettings; i++) {
+            builder.put("node.attr" + i, "value" + i);
+            expectedAttributes.put("attr" + i, "value" + i);
+        }
+        DiscoveryNodeService discoveryNodeService = new DiscoveryNodeService(builder.build());
+        int numCustomAttributes = randomIntBetween(0, 5);
+        Map<String, String> customAttributes = new HashMap<>();
+        for (int i = 0; i < numCustomAttributes; i++) {
+            customAttributes.put("custom-" + randomAsciiOfLengthBetween(5, 10), randomAsciiOfLengthBetween(1, 10));
+        }
+        expectedAttributes.putAll(customAttributes);
+        discoveryNodeService.addCustomAttributeProvider(() -> customAttributes);
+
+        Map<String, String> attributes = discoveryNodeService.buildAttributes();
+        assertThat(attributes, equalTo(expectedAttributes));
+    }
+}

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

@@ -656,19 +656,20 @@ public final class InternalTestCluster extends TestCluster {
      */
     public synchronized Client clientNodeClient() {
         ensureOpen();
-        NodeAndClient randomNodeAndClient = getRandomNodeAndClient(new ClientNodePredicate());
+        NodeAndClient randomNodeAndClient = getRandomNodeAndClient(new NoDataNoMasterNodePredicate());
         if (randomNodeAndClient != null) {
             return randomNodeAndClient.client(random);
         }
         int nodeId = nextNodeId.getAndIncrement();
         Settings settings = getSettings(nodeId, random.nextLong(), Settings.EMPTY);
         startNodeClient(settings);
-        return getRandomNodeAndClient(new ClientNodePredicate()).client(random);
+        return getRandomNodeAndClient(new NoDataNoMasterNodePredicate()).client(random);
     }
 
     public synchronized Client startNodeClient(Settings settings) {
         ensureOpen(); // currently unused
-        Builder builder = settingsBuilder().put(settings).put(Node.NODE_CLIENT_SETTING.getKey(), true);
+        Builder builder = settingsBuilder().put(settings).put(Node.NODE_MASTER_SETTING.getKey(), false)
+            .put(Node.NODE_DATA_SETTING.getKey(), false);
         if (size() == 0) {
             // if we are the first node - don't wait for a state
             builder.put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), 0);
@@ -952,7 +953,8 @@ public final class InternalTestCluster extends TestCluster {
             NodeAndClient nodeAndClient = nodes.get(buildNodeName);
             if (nodeAndClient == null) {
                 changed = true;
-                Builder clientSettingsBuilder = Settings.builder().put(Node.NODE_CLIENT_SETTING.getKey(), true);
+                Builder clientSettingsBuilder = Settings.builder().put(Node.NODE_MASTER_SETTING.getKey(), false)
+                    .put(Node.NODE_DATA_SETTING.getKey(), false);
                 nodeAndClient = buildNode(i, sharedNodesSeeds[i], clientSettingsBuilder.build(), Version.CURRENT);
                 nodeAndClient.node.start();
                 logger.info("Start Shared Node [{}] not shared", nodeAndClient.name);
@@ -1677,10 +1679,11 @@ public final class InternalTestCluster extends TestCluster {
         }
     }
 
-    private static final class ClientNodePredicate implements Predicate<NodeAndClient> {
+    private static final class NoDataNoMasterNodePredicate implements Predicate<NodeAndClient> {
         @Override
         public boolean test(NodeAndClient nodeAndClient) {
-            return DiscoveryNode.clientNode(nodeAndClient.node.settings());
+            return DiscoveryNode.masterNode(nodeAndClient.node.settings()) == false &&
+                DiscoveryNode.dataNode(nodeAndClient.node.settings()) == false;
         }
     }