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.

Sign up for GitHub

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 a new REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' #2404

Merged
merged 14 commits into from
Mar 18, 2022
Merged

Add a new REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' #2404

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a0c43a3
Add _cat cluster_manager as the ReplacedRoute for _cat master rest api
Mar 3, 2022
b23c566
Add yaml rest test for cat cluster manager api
Mar 8, 2022
a33d574
Remove an unused import
Mar 8, 2022
bb79c85
Align the text in yml
Mar 8, 2022
c6fe237
Add a comment to indicate the removal in the future.
Mar 9, 2022
5a751e6
Deprecate path cat master in rest api spec
Mar 11, 2022
c9c1a0b
Merge remote-tracking branch 'upstream/main' into replace-cat-master-api
Mar 16, 2022
be44736
Rename master to cluster-manager in cat.cluster_manager.json
Mar 16, 2022
00a6ff6
Merge branch 'main' into replace-cat-master-api
Mar 16, 2022
466b083
Remove the newly created cat.cluster_manager.json
Mar 16, 2022
a17afa7
Remove the test for newly created cat.cluster_manager.json
Mar 16, 2022
c46e535
Rename cat.master rest-api-spec to cat.cluster_manager
Mar 16, 2022
0b7b236
Fix yaml rest test by remove cat.master
Mar 17, 2022
300f18b
put allowed_warnings in setup section
Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions rest-api-spec/src/main/resources/rest-api-spec/api/cat.cluster_manager.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
{
"cat.cluster_manager":{
"documentation":{
"url":"https://opensearch.org/docs/latest/opensearch/rest-api/cat/cat-master/",
kartg marked this conversation as resolved.
Show resolved Hide resolved
"description":"Returns information about the cluster manager node."
},
"stability":"stable",
"url":{
"paths":[
{
"path":"/_cat/cluster_manager",
"methods":[
"GET"
]
}
]
},
"params":{
"format":{
"type":"string",
"description":"a short version of the Accept header, e.g. json, yaml"
},
"local":{
"type":"boolean",
"description":"Return local information, do not retrieve the state from master node (default: false)"
kartg marked this conversation as resolved.
Show resolved Hide resolved
},
"master_timeout":{
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this parameter name be changed 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.

Hmmm, good question. I didn't think of it at that time. Since the deprecation of the parameter "master_timeout" will be handled in separate PR, maybe it is fair to keep it for now. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should introduce a new API with old parameter names. If a client moves to the _cat/cluster_manager API with this in place, they will continue to use the master_timeout parameter name. Then, when we deprecate the parameter, they will need to make another change to prevent breakage.

I would rather we use inclusive naming from the get-go in net-new APIs and code paths.

Copy link
Collaborator Author

@tlfeng tlfeng Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I got it. 😄 Make sense not to introduce a new API with out-dated parameter name.
I realized that a new json file of rest-api-spec may not necessary, because the main code change I made is to give an alternative API path to _cat/master, but not adding a new API.
I will modify the code not to create a new json file of rest-api-spec, and find out a better way to deal with the API path change.
If that works, I will also modify the PR title and description, because the code change is not actually adding a new REST API.

Copy link
Collaborator Author

@tlfeng tlfeng Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I modified the code to:

  • Rename the current JSON rest-api-spec file cat.master.json to cat.cluster_manager.json
  • Add path _cat/cluster_manager and _cat/master path as the deprecated path
  • Test both old and new path in one YAML file to test the both REST API path

If I should deprecate the request parameter master_timeout, I will make the code change after the PR #2435 merged, because that PR will add the method to validate whether the value of master_timeout and cluster_manager_timeout is equal, and also set a sample for all the master_timeout deprecation.

Copy link
Member

Choose a reason for hiding this comment

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

👍 please add an issue so we don't miss out on it

Copy link
Collaborator Author

@tlfeng tlfeng Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I created the issue #2511 to deprecate the old parameter name, and also attached it to #1549 . Thanks

"type":"time",
"description":"Explicit operation timeout for connection to master node"
},
"h":{
"type":"list",
"description":"Comma-separated list of column names to display"
},
"help":{
"type":"boolean",
"description":"Return help information",
"default":false
},
"s":{
"type":"list",
"description":"Comma-separated list of column names or column aliases to sort by"
},
"v":{
"type":"boolean",
"description":"Verbose mode. Display column headers",
"default":false
}
}
}
}
36 changes: 36 additions & 0 deletions rest-api-spec/src/main/resources/rest-api-spec/test/cat.cluster_manager/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
---
"Help":
- skip:
version: " - 1.4.99"
reason: "_cat/cluster_manager is introduced in 2.0.0"
- do:
cat.cluster_manager:
help: true

- match:
$body: |
/^ id .+ \n
host .+ \n
ip .+ \n
node .+ \n

$/

---
"Test cat cluster_manager output":
- skip:
version: " - 1.4.99"
reason: "_cat/cluster_manager is introduced in 2.0.0"
- do:
cat.cluster_manager: {}

- match:
$body: |
/^
( \S+ \s+ # node id
[-\w.]+ \s+ # host name
(\d{1,3}\.){3}\d{1,3} \s+ # ip address
[-\w.]+ # node name
\n
)
$/
39 changes: 39 additions & 0 deletions rest-api-spec/src/main/resources/rest-api-spec/test/cat.master/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
---
"Help":
- skip:
features: allowed_warnings
- do:
cat.master:
help: true
allowed_warnings:
- '[GET /_cat/master] is deprecated! Use [GET /_cat/cluster_manager] instead.'


- match:
$body: |
/^ id .+ \n
host .+ \n
ip .+ \n
node .+ \n

$/

---
"Test cat master output":
- skip:
features: allowed_warnings
- do:
cat.master: {}
allowed_warnings:
- '[GET /_cat/master] is deprecated! Use [GET /_cat/cluster_manager] instead.'

- match:
$body: |
/^
( \S+ \s+ # node id
[-\w.]+ \s+ # host name
(\d{1,3}\.){3}\d{1,3} \s+ # ip address
[-\w.]+ # node name
\n
)
$/
8 changes: 4 additions & 4 deletions server/src/main/java/org/opensearch/rest/action/cat/RestMasterAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,18 @@
public class RestMasterAction extends AbstractCatAction {
Copy link
Member

Choose a reason for hiding this comment

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

Such class name won't be considered as 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.

Hi @owaiskazi19, thanks for your review! The RestMasterAction class is published to maven as Java API (https://opensearch.org/javadocs/1.2.4/OpenSearch/server/build/docs/javadoc/org/opensearch/rest/action/cat/RestMasterAction.html). I didn't plan to rename them in 2.0 version, in case giving clients or plugins which use such classes too much work in upgrading to support 2.0 version. 😄
There is a discussion in the PR #2453, you could take a look at. Renaming the Java APIs is tracked in issue #1684

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Thanks for clearing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are welcome! 😁


@Override
public List<Route> routes() {
return singletonList(new Route(GET, "/_cat/master"));
public List<ReplacedRoute> replacedRoutes() {
return singletonList(new ReplacedRoute(GET, "/_cat/cluster_manager", "/_cat/master"));
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to support this old API forever?
Or is there a EOL defined after couple of major version releases?

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 "_cat/master" API will be removed in a future major version. Although the current plan is to remove in version 3.0, it depends on the users' acceptance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your idea, I added a comment to indicate the deprecated API will be removed in the future.

}

@Override
public String getName() {
return "cat_master_action";
return "cat_cluster_manager_action";
}

@Override
protected void documentation(StringBuilder sb) {
sb.append("/_cat/master\n");
sb.append("/_cat/cluster_manager\n");
}

@Override
Expand Down