-
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 request parameter 'cluster_manager_timeout' and deprecate 'master_timeout' - in Cluster APIs #2658
Add request parameter 'cluster_manager_timeout' and deprecate 'master_timeout' - in Cluster APIs #2658
Changes from 9 commits
f1aea0f
3c0e0fb
5c770eb
1bb15db
1c8a22f
a21578b
a8b20ca
2fb34ed
5b35951
0a541ec
51d640e
a3199b9
430c815
1b383a1
2399800
33556a4
f5d9fc7
4ce930a
58b30d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ | |
import org.opensearch.cluster.health.ClusterHealthStatus; | ||
import org.opensearch.common.Priority; | ||
import org.opensearch.common.Strings; | ||
import org.opensearch.common.logging.DeprecationLogger; | ||
import org.opensearch.rest.BaseRestHandler; | ||
import org.opensearch.rest.RestRequest; | ||
import org.opensearch.rest.action.RestStatusToXContentListener; | ||
|
@@ -56,6 +57,10 @@ | |
|
||
public class RestClusterHealthAction extends BaseRestHandler { | ||
|
||
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterHealthAction.class); | ||
private static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE = | ||
"Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version."; | ||
|
||
@Override | ||
public List<Route> routes() { | ||
return unmodifiableList(asList(new Route(GET, "/_cluster/health"), new Route(GET, "/_cluster/health/{index}"))); | ||
|
@@ -81,7 +86,8 @@ public static ClusterHealthRequest fromRequest(final RestRequest request) { | |
final ClusterHealthRequest clusterHealthRequest = clusterHealthRequest(Strings.splitStringByCommaToArray(request.param("index"))); | ||
clusterHealthRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterHealthRequest.indicesOptions())); | ||
clusterHealthRequest.local(request.paramAsBoolean("local", clusterHealthRequest.local())); | ||
clusterHealthRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterHealthRequest.masterNodeTimeout())); | ||
clusterHealthRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterHealthRequest.masterNodeTimeout())); | ||
parseDeprecatedMasterTimeoutParameter(clusterHealthRequest, request); | ||
clusterHealthRequest.timeout(request.paramAsTime("timeout", clusterHealthRequest.timeout())); | ||
String waitForStatus = request.param("wait_for_status"); | ||
if (waitForStatus != null) { | ||
|
@@ -122,4 +128,19 @@ public boolean canTripCircuitBreaker() { | |
return false; | ||
} | ||
|
||
/** | ||
* Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used. | ||
* It also validates whether the value of 'master_timeout' is the same with 'cluster_manager_timeout'. | ||
* Remove the method along with MASTER_ROLE. | ||
* @deprecated As of 2.0, because promoting inclusive language. | ||
*/ | ||
@Deprecated | ||
private static void parseDeprecatedMasterTimeoutParameter(ClusterHealthRequest clusterHealthRequest, RestRequest request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove in favor of base method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved these issues in above. 😄 |
||
final String deprecatedTimeoutParam = "master_timeout"; | ||
if (request.hasParam(deprecatedTimeoutParam)) { | ||
deprecationLogger.deprecate("cluster_health_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE); | ||
request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout"); | ||
clusterHealthRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterHealthRequest.masterNodeTimeout())); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,10 +80,12 @@ public RestClusterRerouteAction(SettingsFilter settingsFilter) { | |
} | ||
|
||
// TODO: Remove the DeprecationLogger after removing MASTER_ROLE. | ||
// It's used to log deprecation when request parameter 'metric' contains 'master_node'. | ||
// It's used to log deprecation when request parameter 'metric' contains 'master_node', or request parameter 'master_timeout' is used. | ||
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."; | ||
private static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE = | ||
"Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version."; | ||
|
||
@Override | ||
public List<Route> routes() { | ||
|
@@ -143,8 +145,25 @@ public static ClusterRerouteRequest createRequest(RestRequest request) throws IO | |
clusterRerouteRequest.explain(request.paramAsBoolean("explain", clusterRerouteRequest.explain())); | ||
clusterRerouteRequest.timeout(request.paramAsTime("timeout", clusterRerouteRequest.timeout())); | ||
clusterRerouteRequest.setRetryFailed(request.paramAsBoolean("retry_failed", clusterRerouteRequest.isRetryFailed())); | ||
clusterRerouteRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterRerouteRequest.masterNodeTimeout())); | ||
clusterRerouteRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterRerouteRequest.masterNodeTimeout())); | ||
parseDeprecatedMasterTimeoutParameter(clusterRerouteRequest, request); | ||
request.applyContentParser(parser -> PARSER.parse(parser, clusterRerouteRequest, null)); | ||
return clusterRerouteRequest; | ||
} | ||
|
||
/** | ||
* Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used. | ||
* It also validates whether the value of 'master_timeout' is the same with 'cluster_manager_timeout'. | ||
* Remove the method along with MASTER_ROLE. | ||
* @deprecated As of 2.0, because promoting inclusive language. | ||
*/ | ||
@Deprecated | ||
private static void parseDeprecatedMasterTimeoutParameter(ClusterRerouteRequest clusterRerouteRequest, RestRequest request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove in favor of base method |
||
final String deprecatedTimeoutParam = "master_timeout"; | ||
if (request.hasParam(deprecatedTimeoutParam)) { | ||
deprecationLogger.deprecate("cluster_reroute_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE); | ||
request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout"); | ||
clusterRerouteRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterRerouteRequest.masterNodeTimeout())); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,10 +73,12 @@ public RestClusterStateAction(SettingsFilter settingsFilter) { | |
} | ||
|
||
// TODO: Remove the DeprecationLogger after removing MASTER_ROLE. | ||
// It's used to log deprecation when request parameter 'metric' contains 'master_node'. | ||
// It's used to log deprecation when request parameter 'metric' contains 'master_node', or request parameter 'master_timeout' is used. | ||
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."; | ||
private static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE = | ||
"Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version."; | ||
|
||
@Override | ||
public String getName() { | ||
|
@@ -104,7 +106,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC | |
final ClusterStateRequest clusterStateRequest = Requests.clusterStateRequest(); | ||
clusterStateRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterStateRequest.indicesOptions())); | ||
clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); | ||
clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); | ||
clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout())); | ||
parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request); | ||
if (request.hasParam("wait_for_metadata_version")) { | ||
clusterStateRequest.waitForMetadataVersion(request.paramAsLong("wait_for_metadata_version", 0)); | ||
} | ||
|
@@ -186,4 +189,19 @@ static final class Fields { | |
static final String CLUSTER_NAME = "cluster_name"; | ||
} | ||
|
||
/** | ||
* Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used. | ||
* It also validates whether the value of 'master_timeout' is the same with 'cluster_manager_timeout'. | ||
* Remove the method along with MASTER_ROLE. | ||
* @deprecated As of 2.0, because promoting inclusive language. | ||
*/ | ||
@Deprecated | ||
private static void parseDeprecatedMasterTimeoutParameter(ClusterStateRequest clusterStateRequest, RestRequest request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove in favor of base method |
||
final String deprecatedTimeoutParam = "master_timeout"; | ||
if (request.hasParam(deprecatedTimeoutParam)) { | ||
deprecationLogger.deprecate("cluster_state_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE); | ||
request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout"); | ||
clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout())); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ | |
import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; | ||
import org.opensearch.client.Requests; | ||
import org.opensearch.client.node.NodeClient; | ||
import org.opensearch.common.logging.DeprecationLogger; | ||
import org.opensearch.common.settings.Settings; | ||
import org.opensearch.common.xcontent.XContentParser; | ||
import org.opensearch.rest.BaseRestHandler; | ||
|
@@ -51,6 +52,10 @@ | |
|
||
public class RestClusterUpdateSettingsAction extends BaseRestHandler { | ||
|
||
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterUpdateSettingsAction.class); | ||
private static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE = | ||
"Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version."; | ||
|
||
private static final String PERSISTENT = "persistent"; | ||
private static final String TRANSIENT = "transient"; | ||
|
||
|
@@ -69,8 +74,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC | |
final ClusterUpdateSettingsRequest clusterUpdateSettingsRequest = Requests.clusterUpdateSettingsRequest(); | ||
clusterUpdateSettingsRequest.timeout(request.paramAsTime("timeout", clusterUpdateSettingsRequest.timeout())); | ||
clusterUpdateSettingsRequest.masterNodeTimeout( | ||
request.paramAsTime("master_timeout", clusterUpdateSettingsRequest.masterNodeTimeout()) | ||
request.paramAsTime("cluster_manager_timeout", clusterUpdateSettingsRequest.masterNodeTimeout()) | ||
); | ||
parseDeprecatedMasterTimeoutParameter(clusterUpdateSettingsRequest, request); | ||
Map<String, Object> source; | ||
try (XContentParser parser = request.contentParser()) { | ||
source = parser.map(); | ||
|
@@ -94,4 +100,25 @@ protected Set<String> responseParams() { | |
public boolean canTripCircuitBreaker() { | ||
return false; | ||
} | ||
|
||
/** | ||
* Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used. | ||
* It also validates whether the value of 'master_timeout' is the same with 'cluster_manager_timeout'. | ||
* Remove the method along with MASTER_ROLE. | ||
* @deprecated As of 2.0, because promoting inclusive language. | ||
*/ | ||
@Deprecated | ||
private static void parseDeprecatedMasterTimeoutParameter( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove in favor of base method |
||
ClusterUpdateSettingsRequest clusterUpdateSettingsRequest, | ||
RestRequest request | ||
) { | ||
final String deprecatedTimeoutParam = "master_timeout"; | ||
if (request.hasParam(deprecatedTimeoutParam)) { | ||
deprecationLogger.deprecate("cluster_update_setting_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE); | ||
request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout"); | ||
clusterUpdateSettingsRequest.masterNodeTimeout( | ||
request.paramAsTime(deprecatedTimeoutParam, clusterUpdateSettingsRequest.masterNodeTimeout()) | ||
); | ||
} | ||
} | ||
} |
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.
Can we refactor this as a public static method in
BaseRestHandler.java
and abstract it to take a baseMasterNodeRequest
instance so we don't have to duplicate in each of these files?e.g.,
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.
Hi Nick, thanks a lot for your time in the review!
Sure, I will make the suggested change. I realized adding the duplicate method into every
Action
class is a pain, as well as removing in the future, and my idea was avoid adding the API-specific method into the abstract base class.Since it's a temporary method and you agree with it, I will move it into
BaseRestHandler
class. 😁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.
ty!
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.
@nknize I put your suggested change into a separate PR #2670.
Please take a look at when you have time.
I plan to take that PR as a foundation and example for changing other REST APIs. After that PR merged, the codes of deprecating
master_timeout
parameter in the other REST APIs can be rebased and taken the benefit (such as PR #2557 & #2658).I will update this PR after PR #2670 is merged.
Thank you for your good suggestion 😄, it can really reduce the lines of code changes and save my time!
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 duplidated
parseDeprecatedMasterTimeoutParameter()
method is put into abstract base classBaseRestHandler
through PR #2670, and I have updated this PR. 😁