Add request parameter 'cluster_manager_timeout' and deprecate 'master_timeout' - in Cluster APIs #2658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
March 28, 2022 16:14
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
tlfeng
added
enhancement
11 tasks
added 2 commits
March 29, 2022 21:12
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
added 4 commits
March 29, 2022 21:48
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
tlfeng
force-pushed
the
manager-timeout-cluster-api
branch
from
d63046c to
5b35951
Compare
|
✅ Gradle Check success d63046c0398ff054e00d66a9990f0bda9e19a70c |
nknize
requested changes
Mar 30, 2022
| * @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.
Can we refactor this as a public static method in BaseRestHandler.java and abstract it to take a base MasterNodeRequest instance so we don't have to duplicate in each of these files?
e.g.,
/**
* 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
public static void parseDeprecatedMasterTimeoutParameter(MasterNodeRequest mnr, RestRequest request) {
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");
mnr.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, mnr.masterNodeTimeout()));
}
}There was a problem hiding this comment.
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.
@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.
The duplidated parseDeprecatedMasterTimeoutParameter() method is put into abstract base class BaseRestHandler through PR #2670, and I have updated this PR. 😁
| * @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.
remove in favor of base method
There was a problem hiding this comment.
Resolved these issues in above. 😄
| * @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.
remove in favor of base method
| * @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.
remove in favor of base method
| * @deprecated As of 2.0, because promoting inclusive language. | ||
| */ | ||
| @Deprecated | ||
| private static void parseDeprecatedMasterTimeoutParameter( |
There was a problem hiding this comment.
remove in favor of base method
| * @deprecated As of 2.0, because promoting inclusive language. | ||
| */ | ||
| @Deprecated | ||
| private static void parseDeprecatedMasterTimeoutParameter(PendingClusterTasksRequest pendingClusterTasksRequest, RestRequest request) { |
There was a problem hiding this comment.
remove in favor of base method
| * @deprecated As of 2.0, because promoting inclusive language. | ||
| */ | ||
| @Deprecated | ||
| private static void parseDeprecatedMasterTimeoutParameter(PendingClusterTasksRequest pendingClusterTasksRequest, RestRequest request) { |
There was a problem hiding this comment.
remove in favor of base method
5 tasks
added 3 commits
March 30, 2022 13:38
…r class to reduce duplication Signed-off-by: Tianli Feng <[email protected]>
…r class to reduce duplication Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
dreamer-89
approved these changes
Apr 1, 2022
Signed-off-by: Tianli Feng <[email protected]> # Conflicts: # server/src/test/java/org/opensearch/action/RenamedTimeoutRequestParameterTests.java
tlfeng
force-pushed
the
manager-timeout-cluster-api
branch
from
30ed89a to
2399800
Compare
added 2 commits
April 1, 2022 17:04
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
|
❌ Gradle Check failure 30ed89a7ca94bcc365e33241618e6792fa5aefef |
Signed-off-by: Tianli Feng <[email protected]>
|
In log 4053: and The Client test failure will be resolved in #2702 |
|
In log 4055: The Client test failure will be resolved in #2702 |
nknize
approved these changes
Apr 4, 2022
…ster-api Signed-off-by: Tianli Feng <[email protected]>
|
In log 4127: It's reported in issue #1703 |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2658-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7d23b180f77821266f0acd779a9db637739ee0b9
# Push it to GitHub
git push --set-upstream origin backport/backport-2658-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.xThen, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.0 2.0
# Navigate to the new working tree
cd .worktrees/backport-2.0
# Create a new branch
git switch --create backport/backport-2658-to-2.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7d23b180f77821266f0acd779a9db637739ee0b9
# Push it to GitHub
git push --set-upstream origin backport/backport-2658-to-2.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.0Then, create a pull request where the |
tlfeng
pushed a commit
to tlfeng/OpenSearch
that referenced
this pull request
Apr 4, 2022
…_timeout' - in Cluster APIs (opensearch-project#2658) - Deprecate the request parameter `master_timeout` that used in Cluster APIs which have got the parameter. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]>
tlfeng
pushed a commit
to tlfeng/OpenSearch
that referenced
this pull request
Apr 4, 2022
…_timeout' - in Cluster APIs (opensearch-project#2658) - Deprecate the request parameter `master_timeout` that used in Cluster APIs which have got the parameter. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]>
tlfeng
pushed a commit
that referenced
this pull request
Apr 4, 2022
…_timeout' - in Cluster APIs (#2658) (#2755) - Deprecate the request parameter `master_timeout` that used in Cluster APIs which have got the parameter. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]>
tlfeng
pushed a commit
that referenced
this pull request
Apr 5, 2022
…_timeout' - in Cluster APIs (#2658) (#2754) - Deprecate the request parameter `master_timeout` that used in Cluster APIs which have got the parameter. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Apply the change of CAT Nodes API in PR #2435 to applicable Cluster APIs.
master_timeoutthat used in Cluster APIs which have got the parameter.cluster_manager_timeout.List of the
ClusterAPIs that have got request parametermaster_timeout:Cluster health API https://opensearch.org/docs/opensearch/rest-api/cluster-health/
Cluster reroute API -
POST /_cluster/rerouteCluster state API -
GET /_cluster/state/<metrics>/<target>Cluster get settings API https://opensearch.org/docs/latest/opensearch/rest-api/cluster-settings/
Cluster update settings API https://opensearch.org/docs/latest/opensearch/rest-api/cluster-settings/
Pending cluster tasks API -
GET /_cluster/pending_tasksIssues Resolved
A part of issue #2511
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.