Skip to content

Commit

Permalink
[Transform] Remove node.attr.transform.remote_connect and use new rem…
Browse files Browse the repository at this point in the history 
…ote cluster client node role (elastic#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 elastic#54179
  • Loading branch information
@benwtrent
benwtrent committed Mar 25, 2020
1 parent b8b7516 commit 66127d0
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 37 deletions.
6 changes: 1 addition & 5 deletions x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -367,9 +365,8 @@ public List<Setting<?>> 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"
);
Expand All @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion ...n/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
19 changes: 2 additions & 17 deletions x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/TransformTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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")
Expand Down
30 changes: 16 additions & 14 deletions ...a/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ public void testNodeAssignmentProblems() {
public void testDoNotSelectOldNodes() {
Map<String, String> 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);
Expand Down Expand Up @@ -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
)
)
Expand Down Expand Up @@ -358,18 +361,13 @@ private DiscoveryNodes.Builder buildNodes(
boolean dedicatedTransformNode,
boolean pastDataNode,
boolean transformRemoteNodes,
boolean transformLocanOnlyNodes,
boolean transformLocalOnlyNodes,
boolean currentDataNode
) {
Map<String, String> transformNodeAttributes = new HashMap<>();
transformNodeAttributes.put(Transform.TRANSFORM_ENABLED_NODE_ATTR, "true");
transformNodeAttributes.put(Transform.TRANSFORM_REMOTE_ENABLED_NODE_ATTR, "true");
Map<String, String> transformNodeAttributesDisabled = new HashMap<>();
transformNodeAttributesDisabled.put(Transform.TRANSFORM_ENABLED_NODE_ATTR, "false");
transformNodeAttributesDisabled.put(Transform.TRANSFORM_REMOTE_ENABLED_NODE_ATTR, "true");
Map<String, String> 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();

Expand All @@ -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
)
);
Expand All @@ -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
)
)
Expand All @@ -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
)
Expand All @@ -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
)
);
Expand Down

0 comments on commit 66127d0

Please sign in to comment.