From 151d05b870b5c0d8f27c107a1a287c34b05c4f6c Mon Sep 17 00:00:00 2001 From: Benjamin Trent Date: Wed, 25 Mar 2020 15:05:08 -0400 Subject: [PATCH] [Transform] Remove node.attr.transform.remote_connect and use new remote cluster client node role (#54217) With the addition of a formal role for nodes indicating remote cluster connection, the transform specific attribute `node.attr.transform.remote_connect` is no longer necessary. closes https://github.com/elastic/elasticsearch/issues/54179 --- .../xpack/transform/Transform.java | 6 +--- .../TransformPersistentTasksExecutor.java | 2 +- .../xpack/transform/TransformTests.java | 19 ++---------- ...TransformPersistentTasksExecutorTests.java | 30 ++++++++++--------- 4 files changed, 20 insertions(+), 37 deletions(-) diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java index 37bcdc7786a9b..e84dbe1b0a760 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java @@ -34,7 +34,6 @@ import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.indices.SystemIndexDescriptor; import org.elasticsearch.license.XPackLicenseState; -import org.elasticsearch.node.Node; import org.elasticsearch.persistent.PersistentTasksExecutor; import org.elasticsearch.plugins.PersistentTaskPlugin; import org.elasticsearch.plugins.Plugin; @@ -150,7 +149,6 @@ public class Transform extends Plugin implements SystemIndexPlugin, PersistentTa * These attributes should never be set directly, use the node setting counter parts instead. */ public static final String TRANSFORM_ENABLED_NODE_ATTR = "transform.node"; - public static final String TRANSFORM_REMOTE_ENABLED_NODE_ATTR = "transform.remote_connect"; /** * Setting whether transform (the coordinator task) can run on this node and REST API's are available, @@ -367,9 +365,8 @@ public List> getSettings() { @Override public Settings additionalSettings() { String transformEnabledNodeAttribute = "node.attr." + TRANSFORM_ENABLED_NODE_ATTR; - String transformRemoteEnabledNodeAttribute = "node.attr." + TRANSFORM_REMOTE_ENABLED_NODE_ATTR; - if (settings.get(transformEnabledNodeAttribute) != null || settings.get(transformRemoteEnabledNodeAttribute) != null) { + if (settings.get(transformEnabledNodeAttribute) != null) { throw new IllegalArgumentException( "Directly setting transform node attributes is not permitted, please use the documented node settings instead" ); @@ -382,7 +379,6 @@ public Settings additionalSettings() { Settings.Builder additionalSettings = Settings.builder(); additionalSettings.put(transformEnabledNodeAttribute, TRANSFORM_ENABLED_NODE.get(settings)); - additionalSettings.put(transformRemoteEnabledNodeAttribute, Node.NODE_REMOTE_CLUSTER_CLIENT.get(settings)); return additionalSettings.build(); } diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutor.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutor.java index cd250bf480b10..8e31040dace82 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutor.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutor.java @@ -186,7 +186,7 @@ public static boolean nodeCanRunThisTransform(DiscoveryNode node, TransformTaskP } // does the transform require a remote and remote is enabled? - if (params.requiresRemote() && Boolean.parseBoolean(nodeAttributes.get(Transform.TRANSFORM_REMOTE_ENABLED_NODE_ATTR)) == false) { + if (params.requiresRemote() && node.isRemoteClusterClient() == false) { if (explain != null) { explain.put(node.getId(), "transform requires a remote connection but remote is disabled"); } diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/TransformTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/TransformTests.java index 2ca4b11b43dd5..49dd17d8f9e59 100644 --- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/TransformTests.java +++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/TransformTests.java @@ -19,18 +19,12 @@ public void testNodeAttributes() { Settings.Builder builder = Settings.builder(); boolean transformEnabled = randomBoolean(); boolean transformPluginEnabled = randomBoolean(); - boolean remoteClusterClient = randomBoolean(); // randomly use explicit or default setting if ((transformEnabled && randomBoolean()) == false) { builder.put("node.transform", transformEnabled); } - // randomly use explicit or default setting - if ((remoteClusterClient && randomBoolean()) == false) { - builder.put("node.remote_cluster_client", remoteClusterClient); - } - if (transformPluginEnabled == false) { builder.put("xpack.transform.enabled", transformPluginEnabled); } @@ -42,23 +36,14 @@ public void testNodeAttributes() { transformPluginEnabled && transformEnabled, Boolean.parseBoolean(transform.additionalSettings().get("node.attr.transform.node")) ); - assertEquals( - transformPluginEnabled && remoteClusterClient, - Boolean.parseBoolean(transform.additionalSettings().get("node.attr.transform.remote_connect")) - ); } public void testNodeAttributesDirectlyGiven() { Settings.Builder builder = Settings.builder(); - - if (randomBoolean()) { - builder.put("node.attr.transform.node", randomBoolean()); - } else { - builder.put("node.attr.transform.remote_connect", randomBoolean()); - } + builder.put("node.attr.transform.node", randomBoolean()); Transform transform = createTransform(builder.build()); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> transform.additionalSettings()); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, transform::additionalSettings); assertThat( e.getMessage(), equalTo("Directly setting transform node attributes is not permitted, please use the documented node settings instead") diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java index dd3be79d5a8d9..9e90d005ff90b 100644 --- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java +++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java @@ -170,7 +170,6 @@ public void testNodeAssignmentProblems() { public void testDoNotSelectOldNodes() { Map transformNodeAttributes = new HashMap<>(); transformNodeAttributes.put(Transform.TRANSFORM_ENABLED_NODE_ATTR, "true"); - transformNodeAttributes.put(Transform.TRANSFORM_REMOTE_ENABLED_NODE_ATTR, "true"); MetaData.Builder metaData = MetaData.builder(); RoutingTable.Builder routingTable = RoutingTable.builder(); addIndices(metaData, routingTable); @@ -201,7 +200,11 @@ public void testDoNotSelectOldNodes() { "current-data-node-with-1-task", buildNewFakeTransportAddress(), transformNodeAttributes, - new HashSet<>(Arrays.asList(DiscoveryNodeRole.DATA_ROLE, DiscoveryNodeRole.MASTER_ROLE)), + new HashSet<>(Arrays.asList( + DiscoveryNodeRole.DATA_ROLE, + DiscoveryNodeRole.MASTER_ROLE, + DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE + )), Version.CURRENT ) ) @@ -358,18 +361,13 @@ private DiscoveryNodes.Builder buildNodes( boolean dedicatedTransformNode, boolean pastDataNode, boolean transformRemoteNodes, - boolean transformLocanOnlyNodes, + boolean transformLocalOnlyNodes, boolean currentDataNode ) { Map transformNodeAttributes = new HashMap<>(); transformNodeAttributes.put(Transform.TRANSFORM_ENABLED_NODE_ATTR, "true"); - transformNodeAttributes.put(Transform.TRANSFORM_REMOTE_ENABLED_NODE_ATTR, "true"); Map transformNodeAttributesDisabled = new HashMap<>(); transformNodeAttributesDisabled.put(Transform.TRANSFORM_ENABLED_NODE_ATTR, "false"); - transformNodeAttributesDisabled.put(Transform.TRANSFORM_REMOTE_ENABLED_NODE_ATTR, "true"); - Map transformNodeAttributesNoRemote = new HashMap<>(); - transformNodeAttributesNoRemote.put(Transform.TRANSFORM_ENABLED_NODE_ATTR, "true"); - transformNodeAttributesNoRemote.put(Transform.TRANSFORM_REMOTE_ENABLED_NODE_ATTR, "false"); DiscoveryNodes.Builder nodes = DiscoveryNodes.builder(); @@ -379,7 +377,7 @@ private DiscoveryNodes.Builder buildNodes( "dedicated-transform-node", buildNewFakeTransportAddress(), transformNodeAttributes, - Collections.singleton(DiscoveryNodeRole.MASTER_ROLE), + new HashSet<>(Arrays.asList(DiscoveryNodeRole.MASTER_ROLE, DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE)), Version.CURRENT ) ); @@ -403,7 +401,7 @@ private DiscoveryNodes.Builder buildNodes( "current-data-node-with-2-tasks", buildNewFakeTransportAddress(), transformNodeAttributes, - new HashSet<>(Arrays.asList(DiscoveryNodeRole.DATA_ROLE)), + new HashSet<>(Arrays.asList(DiscoveryNodeRole.DATA_ROLE, DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE)), Version.CURRENT ) ) @@ -412,18 +410,18 @@ private DiscoveryNodes.Builder buildNodes( "current-data-node-with-1-tasks", buildNewFakeTransportAddress(), transformNodeAttributes, - new HashSet<>(Arrays.asList(DiscoveryNodeRole.MASTER_ROLE)), + new HashSet<>(Arrays.asList(DiscoveryNodeRole.MASTER_ROLE, DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE)), Version.CURRENT ) ); } - if (transformLocanOnlyNodes) { + if (transformLocalOnlyNodes) { nodes.add( new DiscoveryNode( "current-data-node-with-0-tasks-transform-remote-disabled", buildNewFakeTransportAddress(), - transformNodeAttributesNoRemote, + transformNodeAttributes, new HashSet<>(Arrays.asList(DiscoveryNodeRole.DATA_ROLE, DiscoveryNodeRole.MASTER_ROLE)), Version.CURRENT ) @@ -436,7 +434,11 @@ private DiscoveryNodes.Builder buildNodes( "current-data-node-with-transform-disabled", buildNewFakeTransportAddress(), transformNodeAttributesDisabled, - new HashSet<>(Arrays.asList(DiscoveryNodeRole.DATA_ROLE, DiscoveryNodeRole.MASTER_ROLE)), + new HashSet<>(Arrays.asList( + DiscoveryNodeRole.DATA_ROLE, + DiscoveryNodeRole.MASTER_ROLE, + DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE + )), Version.CURRENT ) );