Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'cluster_manager_node' into ClusterState Metric as an alternative to 'master_node' #2415

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"nodes",
"routing_table",
"master_node",
"cluster_manager_node",
"version"
],
"description":"Limit the information returned to the specified metrics. Defaults to all but metadata"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
"routing_table",
"routing_nodes",
"master_node",
"cluster_manager_node",
"version"
],
"description":"Limit the information returned to the specified metrics"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,18 @@
"Basic sanity check":
- do:
cluster.reroute: {}

---
"Cluster reroute returns cluster_manager_node":
- skip:
version: " - 1.4.99"
reason: "The metric cluster_manager_node is added to cluster state in version 2.0.0"

- do:
cluster.reroute: {}

- set:
state.cluster_manager_node: node_id

- match: {state.master_node: $node_id}
- match: {state.cluster_manager_node: $node_id}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
- do:
cluster.reroute: {}
- is_false: state.metadata

---
"return metadata if requested":
- do:
Expand All @@ -12,3 +13,15 @@
- is_true: state.metadata
- is_false: state.nodes

---
"Filter the cluster reroute by cluster_manager_node only should work":
- skip:
version: " - 1.4.99"
reason: "The metric cluster_manager_node is added to cluster state in version 2.0.0"

- do:
cluster.reroute:
metric: [ cluster_manager_node ]

- is_true: state.cluster_manager_node
- is_false: state.master_node
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,18 @@

- is_true: cluster_uuid
- is_true: master_node

---
"Get cluster state returns cluster_manager_node":
- skip:
version: " - 1.4.99"
reason: "The metric cluster_manager_node is added to cluster state in version 2.0.0"

- do:
cluster.state: {}

- set:
cluster_manager_node: node_id

- match: {master_node: $node_id}
- match: {cluster_manager_node: $node_id}
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ setup:
- skip:
version: " - 6.3.99"
reason: "cluster state including cluster_uuid at the top level is new in v6.4.0 and higher"
features: allowed_warnings

# Get the current cluster_uuid
- do:
Expand All @@ -167,6 +168,8 @@ setup:
- do:
cluster.state:
metric: [ master_node, version ]
allowed_warnings:
- 'Deprecated value [master_node] used for parameter [metric]. To promote inclusive language, please use [cluster_manager_node] instead. It will be unsupported in a future major version.'

- match: { cluster_uuid: $cluster_uuid }
- is_true: master_node
Expand All @@ -180,3 +183,16 @@ setup:

- match: { cluster_uuid: $cluster_uuid }
- is_true: routing_table

---
"Filter the cluster state by cluster_manager_node only should work":
- skip:
version: " - 1.4.99"
reason: "The metric cluster_manager_node is added to cluster state in version 2.0.0"

- do:
cluster.state:
metric: [ cluster_manager_node ]

- is_true: cluster_manager_node
- is_false: master_node
11 changes: 11 additions & 0 deletions server/src/main/java/org/opensearch/cluster/ClusterState.java
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,13 @@ public boolean supersedes(ClusterState other) {

public enum Metric {
VERSION("version"),

/**
* @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #CLUSTER_MANAGER_NODE}
*/
@Deprecated
MASTER_NODE("master_node"),
CLUSTER_MANAGER_NODE("cluster_manager_node"),
BLOCKS("blocks"),
NODES("nodes"),
METADATA("metadata"),
Expand Down Expand Up @@ -467,6 +473,11 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.field("master_node", nodes().getMasterNodeId());
}

// Value of the field is identical with the above, and aims to replace the above field.
if (metrics.contains(Metric.CLUSTER_MANAGER_NODE)) {
builder.field("cluster_manager_node", nodes().getMasterNodeId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted on a related PR, please create a Github issue to track changing these internal methods to be inclusive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my mind, it's not an internal method, because it's shown in the Javadoc (https://opensearch.org/javadocs/1.3.0/OpenSearch/server/build/docs/javadoc/org/opensearch/cluster/node/DiscoveryNodes.html#getMasterNodeId()).
I think method listed in Javadoc needs an enough period of time to deprecate in advance, before removing.
The issue to track the internal usages is #1548, and there is a PR out. Maybe I'd better update the description to list usages in different file directories.
(Pasted the related PR here #2453 (comment))

}

if (metrics.contains(Metric.BLOCKS)) {
builder.startObject("blocks");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.opensearch.cluster.routing.allocation.command.AllocationCommands;
import org.opensearch.common.ParseField;
import org.opensearch.common.Strings;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.settings.SettingsFilter;
import org.opensearch.common.xcontent.ObjectParser;
Expand Down Expand Up @@ -78,6 +79,12 @@ public RestClusterRerouteAction(SettingsFilter settingsFilter) {
this.settingsFilter = settingsFilter;
}

// TODO: Remove the DeprecationLogger after removing MASTER_ROLE.
// It's used to log deprecation when request parameter 'metric' contains 'master_node'.
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterRerouteAction.class);
private static final String DEPRECATED_MESSAGE_MASTER_NODE =
"Deprecated value [master_node] used for parameter [metric]. To promote inclusive language, please use [cluster_manager_node] instead. It will be unsupported in a future major version.";

@Override
public List<Route> routes() {
return singletonList(new Route(POST, "/_cluster/reroute"));
Expand All @@ -104,6 +111,14 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
final String metric = request.param("metric");
if (metric == null) {
request.params().put("metric", DEFAULT_METRICS);
} else {
// TODO: Remove the statements in 'else' after removing MASTER_ROLE.
EnumSet<ClusterState.Metric> metrics = ClusterState.Metric.parseString(request.param("metric"), true);
// Because "_all" value will add all Metric into metrics set, for prevent deprecation message shown in that case,
// add the check of validating metrics set doesn't contain all enum elements.
if (!metrics.equals(EnumSet.allOf(ClusterState.Metric.class)) && metrics.contains(ClusterState.Metric.MASTER_NODE)) {
deprecationLogger.deprecate("cluster_reroute_metric_parameter_master_node_value", DEPRECATED_MESSAGE_MASTER_NODE);
}
}
return channel -> client.admin().cluster().reroute(clusterRerouteRequest, new RestToXContentListener<>(channel));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding - are there no changes needed here to support/parse the new cluster_manager_node parameter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new cluster_manager_node is a value of the request parameter metric. I think the only code to support is on above, where you posted a comment 😄.
Since it worked, I haven't revise the whole process of the parameter parsing.

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.opensearch.cluster.ClusterState;
import org.opensearch.cluster.metadata.Metadata;
import org.opensearch.common.Strings;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.settings.SettingsFilter;
import org.opensearch.common.xcontent.ToXContent;
Expand Down Expand Up @@ -71,6 +72,12 @@ public RestClusterStateAction(SettingsFilter settingsFilter) {
this.settingsFilter = settingsFilter;
}

// TODO: Remove the DeprecationLogger after removing MASTER_ROLE.
// It's used to log deprecation when request parameter 'metric' contains 'master_node'.
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterStateAction.class);
private static final String DEPRECATED_MESSAGE_MASTER_NODE =
"Deprecated value [master_node] used for parameter [metric]. To promote inclusive language, please use [cluster_manager_node] instead. It will be unsupported in a future major version.";

@Override
public String getName() {
return "cluster_state_action";
Expand Down Expand Up @@ -112,7 +119,17 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
if (request.hasParam("metric")) {
EnumSet<ClusterState.Metric> metrics = ClusterState.Metric.parseString(request.param("metric"), true);
// do not ask for what we do not need.
clusterStateRequest.nodes(metrics.contains(ClusterState.Metric.NODES) || metrics.contains(ClusterState.Metric.MASTER_NODE));
clusterStateRequest.nodes(
metrics.contains(ClusterState.Metric.NODES)
|| metrics.contains(ClusterState.Metric.MASTER_NODE)
|| metrics.contains(ClusterState.Metric.CLUSTER_MANAGER_NODE)
);
// TODO: Remove the DeprecationLogger after removing MASTER_ROLE.
// Because "_all" value will add all Metric into metrics set, for prevent deprecation message shown in that case,
// add the check of validating metrics set doesn't contain all enum elements.
if (!metrics.equals(EnumSet.allOf(ClusterState.Metric.class)) && metrics.contains(ClusterState.Metric.MASTER_NODE)) {
deprecationLogger.deprecate("cluster_state_metric_parameter_master_node_value", DEPRECATED_MESSAGE_MASTER_NODE);
}
/*
* there is no distinction in Java api between routing_table and routing_nodes, it's the same info set over the wire, one single
* flag to ask for it
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ public void testToXContent() throws IOException {
+ clusterState.stateUUID()
+ "\",\n"
+ " \"master_node\" : \"node0\",\n"
+ " \"cluster_manager_node\" : \"node0\",\n"
+ " \"blocks\" : { },\n"
+ " \"nodes\" : {\n"
+ " \"node0\" : {\n"
Expand Down Expand Up @@ -173,7 +174,7 @@ public void testToXContent() throws IOException {
XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint();
Map<String, String> params = new HashMap<>();
params.put("explain", "true");
params.put("metric", "version,master_node");
params.put("metric", "version,cluster_manager_node");
clusterRerouteResponse.toXContent(builder, new ToXContent.MapParams(params));
assertEquals(
"{\n"
Expand All @@ -184,7 +185,7 @@ public void testToXContent() throws IOException {
+ " \"state_uuid\" : \""
+ clusterState.stateUUID()
+ "\",\n"
+ " \"master_node\" : \"node0\"\n"
+ " \"cluster_manager_node\" : \"node0\"\n"
+ " },\n"
+ " \"explanations\" : [\n"
+ " {\n"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ public void testToXContent() throws IOException {
+ " \"version\" : 0,\n"
+ " \"state_uuid\" : \"stateUUID\",\n"
+ " \"master_node\" : \"masterNodeId\",\n"
+ " \"cluster_manager_node\" : \"masterNodeId\",\n"
+ " \"blocks\" : {\n"
+ " \"global\" : {\n"
+ " \"1\" : {\n"
Expand Down Expand Up @@ -352,6 +353,7 @@ public void testToXContent_FlatSettingTrue_ReduceMappingFalse() throws IOExcepti
+ " \"version\" : 0,\n"
+ " \"state_uuid\" : \"stateUUID\",\n"
+ " \"master_node\" : \"masterNodeId\",\n"
+ " \"cluster_manager_node\" : \"masterNodeId\",\n"
+ " \"blocks\" : {\n"
+ " \"global\" : {\n"
+ " \"1\" : {\n"
Expand Down Expand Up @@ -550,6 +552,7 @@ public void testToXContent_FlatSettingFalse_ReduceMappingTrue() throws IOExcepti
+ " \"version\" : 0,\n"
+ " \"state_uuid\" : \"stateUUID\",\n"
+ " \"master_node\" : \"masterNodeId\",\n"
+ " \"cluster_manager_node\" : \"masterNodeId\",\n"
+ " \"blocks\" : {\n"
+ " \"global\" : {\n"
+ " \"1\" : {\n"
Expand Down Expand Up @@ -772,6 +775,7 @@ public void testToXContentSameTypeName() throws IOException {
+ " \"version\" : 0,\n"
+ " \"state_uuid\" : \"stateUUID\",\n"
+ " \"master_node\" : null,\n"
+ " \"cluster_manager_node\" : null,\n"
+ " \"blocks\" : { },\n"
+ " \"nodes\" : { },\n"
+ " \"metadata\" : {\n"
Expand Down