From eb941d8005c45af402950cc37df3527abbe71d6b Mon Sep 17 00:00:00 2001 From: javanna Date: Thu, 3 Mar 2016 08:09:54 +0100 Subject: [PATCH] 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. --- .../support/ThreadedActionListener.java | 7 +-- .../client/transport/TransportClient.java | 5 +- .../cluster/node/DiscoveryNode.java | 41 ++---------- .../cluster/node/DiscoveryNodeService.java | 14 ++--- .../common/settings/ClusterSettings.java | 1 - .../discovery/zen/ZenDiscovery.java | 9 +-- .../java/org/elasticsearch/node/Node.java | 3 +- .../org/elasticsearch/tribe/TribeService.java | 8 ++- .../action/ListenerActionIT.java | 3 +- .../node/DiscoveryNodeServiceTests.java | 63 +++++++++++++++++++ .../test/InternalTestCluster.java | 15 +++-- 11 files changed, 104 insertions(+), 65 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java diff --git a/core/src/main/java/org/elasticsearch/action/support/ThreadedActionListener.java b/core/src/main/java/org/elasticsearch/action/support/ThreadedActionListener.java index 6ed52e1ac46..1eec4c8d9e9 100644 --- a/core/src/main/java/org/elasticsearch/action/support/ThreadedActionListener.java +++ b/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 implements ActionListener ActionListener wrap(ActionListener listener) { diff --git a/core/src/main/java/org/elasticsearch/client/transport/TransportClient.java b/core/src/main/java/org/elasticsearch/client/transport/TransportClient.java index 87a8ca53efd..39d4a2f1e60 100644 --- a/core/src/main/java/org/elasticsearch/client/transport/TransportClient.java +++ b/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); } diff --git a/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java b/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java index 03506643eeb..fdecfe39e45 100644 --- a/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java +++ b/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; - } - - public static boolean clientNode(Settings settings) { - return Node.NODE_CLIENT_SETTING.get(settings); + return Node.NODE_DATA_SETTING.get(settings) || Node.NODE_MASTER_SETTING.get(settings); } public static boolean masterNode(Settings settings) { - if (Node.NODE_MASTER_SETTING.exists(settings)) { - return Node.NODE_MASTER_SETTING.get(settings); - } - return clientNode(settings) == false; + return Node.NODE_MASTER_SETTING.get(settings); } public static boolean dataNode(Settings settings) { - if (Node.NODE_DATA_SETTING.exists(settings)) { - return Node.NODE_DATA_SETTING.get(settings); - } - return clientNode(settings) == false; + return Node.NODE_DATA_SETTING.get(settings); } 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 !clientNode(); - } - return Booleans.parseBooleanExact(data); + return data == null ? true : 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); - if (master == null) { - return !clientNode(); - } - return Booleans.parseBooleanExact(master); + String master = attributes.get(MASTER_ATTR); + return master == null ? true : Booleans.parseBooleanExact(master); } /** diff --git a/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeService.java b/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeService.java index 83f603d2890..c237b78e98e 100644 --- a/core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodeService.java +++ b/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 buildAttributes() { - Map attributes = new HashMap<>(settings.getByPrefix("node.").getAsMap()); + Map 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")) { - attributes.remove("client"); // this is the default - } else { - // if we are client node, don't store data ... - attributes.put("data", "false"); - } + throw new IllegalArgumentException("node.client setting is no longer supported, use " + Node.NODE_MASTER_SETTING.getKey() + + ", " + Node.NODE_DATA_SETTING.getKey() + " and " + Node.NODE_INGEST_SETTING.getKey() + " explicitly instead"); } + //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 buildAttributes(); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index fa8b8c4ac41..9d7fc660cfc 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/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, diff --git a/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java b/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java index fb0f7a61966..ab5d61e7aa0 100644 --- a/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java +++ b/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java @@ -853,10 +853,11 @@ public class ZenDiscovery extends AbstractLifecycleComponent implemen List pingResponses = new ArrayList<>(); for (ZenPing.PingResponse pingResponse : fullPingResponses) { DiscoveryNode node = pingResponse.node(); - if (masterElectionFilterClientNodes && (node.clientNode() || (!node.masterNode() && !node.dataNode()))) { - // filter out the client node, which is a client node, or also one that is not data and not master (effectively, client) - } else if (masterElectionFilterDataNodes && (!node.masterNode() && node.dataNode())) { - // filter out data node that is not also master + //nocommit we should rename this and its setting, also we ignore node.ingest, but maybe it's ok here + if (masterElectionFilterClientNodes && node.masterNode() == false && node.dataNode() == false) { + // filter out nodes that don't hold data and are not master eligible + } else if (masterElectionFilterDataNodes && node.masterNode() == false && node.dataNode()) { + // filter out dedicated data nodes } else { pingResponses.add(pingResponse); } diff --git a/core/src/main/java/org/elasticsearch/node/Node.java b/core/src/main/java/org/elasticsearch/node/Node.java index d115fbb8773..a78a08ef32a 100644 --- a/core/src/main/java/org/elasticsearch/node/Node.java +++ b/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 WRITE_PORTS_FIELD_SETTING = Setting.boolSetting("node.portsfile", false, false, Setting.Scope.CLUSTER); - public static final Setting NODE_CLIENT_SETTING = Setting.boolSetting("node.client", false, false, Setting.Scope.CLUSTER); public static final Setting NODE_DATA_SETTING = Setting.boolSetting("node.data", true, false, Setting.Scope.CLUSTER); public static final Setting NODE_MASTER_SETTING = Setting.boolSetting("node.master", true, false, Setting.Scope.CLUSTER); public static final Setting NODE_LOCAL_SETTING = Setting.boolSetting("node.local", false, false, Setting.Scope.CLUSTER); public static final Setting NODE_MODE_SETTING = new Setting<>("node.mode", "network", Function.identity(), false, Setting.Scope.CLUSTER); public static final Setting NODE_INGEST_SETTING = Setting.boolSetting("node.ingest", true, false, Setting.Scope.CLUSTER); public static final Setting 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 NODE_ATTRIBUTES = Setting.groupSetting("node.", false, Setting.Scope.CLUSTER); diff --git a/core/src/main/java/org/elasticsearch/tribe/TribeService.java b/core/src/main/java/org/elasticsearch/tribe/TribeService.java index bf66cce1b9e..6f30a4931ab 100644 --- a/core/src/main/java/org/elasticsearch/tribe/TribeService.java +++ b/core/src/main/java/org/elasticsearch/tribe/TribeService.java @@ -107,7 +107,9 @@ public class TribeService extends AbstractLifecycleComponent { } // 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 { 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())); } diff --git a/core/src/test/java/org/elasticsearch/action/ListenerActionIT.java b/core/src/test/java/org/elasticsearch/action/ListenerActionIT.java index a9bb96a0a9f..3976f665df9 100644 --- a/core/src/test/java/org/elasticsearch/action/ListenerActionIT.java +++ b/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 { diff --git a/core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java b/core/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeServiceTests.java new file mode 100644 index 00000000000..fda2b6a3f47 --- /dev/null +++ b/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 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 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 attributes = discoveryNodeService.buildAttributes(); + assertThat(attributes, equalTo(expectedAttributes)); + } +} diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index 04548eb85c9..51fab2d2f4f 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/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 { + private static final class NoDataNoMasterNodePredicate implements Predicate { @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; } }