-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add 'cluster_manager_node' into ClusterState Metric as an alternative to 'master_node' #2415
Conversation
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Can one of the admins verify this patch? |
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Signed-off-by: Tianli Feng <ftianli@amazon.com>
In Log 3195:
It's reported in issue #1565, and can't be reproduced locally. |
start gradle check |
In log 3196:
It's reported in issue #2359, and not reproducible locally. |
❌ Gradle Check failure 56b77a78705bebe6714482c1544041971e0456f6 |
Signed-off-by: Tianli Feng <ftl94@live.com>
56b77a7
to
d2c177a
Compare
In log 3200:
It's reported in issue #1746, and isn't reproducible locally. |
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Signed-off-by: Tianli Feng <ftianli@amazon.com>
In log 3314:
Created issue for the last failure #2502 |
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Signed-off-by: Tianli Feng <ftianli@amazon.com>
9c789ab
to
ac29883
Compare
❌ Gradle Check failure 9c789ab78b03844087e9cf1cf8bc8463b9c5a51f |
In log 3332:
It's reproduced in issue #2440 |
Signed-off-by: Tianli Feng <ftianli@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM
@@ -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()); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))
// 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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Description
cluster_manager_node
into ClusterState Metric, as an alternative tomaster_node
. So that the request parameter "metric" inCluster reroute
andCluster state
API accept the new valuecluster_manager_node
MASTER_NODE("master_node")
Note: the metric value "master_node" will be removed in a future major version.
Current API response:
Proposed API response:
Testing:
Issues Resolved
Part of #1549
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.