Add a new REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' by tlfeng · Pull Request #2404 · opensearch-project/OpenSearch

tlfeng · 2022-03-08T07:27:32Z

Description

Add a new REST API endpoint GET _cat/cluster_manager, as alternative of the existing GET _cat/master

Add a new path /_cat/cluster_manager for the REST request handler RestMasterAction
Deprecate the existing path /_cat/master
Change the name of the RestMasterAction handler from cat_master_action to cat_cluster_manager_action
Change the response of GET _cat to only show the new path /_cat/cluster_manager
Rename the JSON rest-api-spec file cat.master.json to cat.cluster_manager.json and update the paths.
Add YAML REST test. By default, it will run test for both endpoints GET _cat/cluster_manager and GET _cat/master

Current API response of "GET _cat"

=^.^=
/_cat/allocation
/_cat/shards
/_cat/shards/{index}
/_cat/master
...

Proposed API response of "GET _cat"

=^.^=
/_cat/allocation
/_cat/shards
/_cat/shards/{index}
/_cat/cluster_manager
...

Testing
running YAML REST test

./gradlew ':rest-api-spec:yamlRestTest' \
  --tests "org.opensearch.test.rest.ClientYamlTestSuiteIT" \
  -Dtests.method="test {p0=cat.cluster_manager/10_basic/*}"

Issues Resolved

Part of #1549

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-08T07:28:03Z

Can one of the admins verify this patch?

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-08T07:43:01Z

❌ Gradle Check failure b23c566
Log 3161

Reports 3161

opensearch-ci-bot · 2022-03-08T08:10:59Z

✅ Gradle Check success a33d574
Log 3162

Reports 3162

saratvemulapalli · 2022-03-08T18:02:37Z

server/src/main/java/org/opensearch/rest/action/cat/RestMasterAction.java

-    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"));


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

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.

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

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-08T22:41:26Z

❌ Gradle Check failure bb79c85
Log 3188

Reports 3188

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-09T01:50:50Z

✅ Gradle Check success c6fe237
Log 3194

Reports 3194

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-11T08:08:50Z

✅ Gradle Check success 5a751e6
Log 3260

Reports 3260

rest-api-spec/src/main/resources/rest-api-spec/api/cat.cluster_manager.json

kartg · 2022-03-16T00:20:26Z

rest-api-spec/src/main/resources/rest-api-spec/api/cat.cluster_manager.json

+        "type":"boolean",
+        "description":"Return local information, do not retrieve the state from master node (default: false)"
+      },
+      "master_timeout":{


Shouldn't this parameter name be changed to be inclusive?

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. 🙂

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.

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.

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.

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

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

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-16T06:38:46Z

❌ Gradle Check failure be44736
Log 3435

Reports 3435

tlfeng · 2022-03-16T16:35:08Z

In log 3435:

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.search.simple.SimpleSearchIT.testSimpleIndexSortEarlyTerminate" -Dtests.seed=FA0A526C8C721A5F -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=el -Dtests.timezone=Pacific/Pitcairn -Druntime.java=17

org.opensearch.search.simple.SimpleSearchIT > testSimpleIndexSortEarlyTerminate FAILED
    java.lang.AssertionError: at least one shard failed CheckIndex
        at __randomizedtesting.SeedInfo.seed([FA0A526C8C721A5F:32A4955E75677464]:0)
        at org.opensearch.test.OpenSearchTestCase.ensureCheckIndexPassed(OpenSearchTestCase.java:663)
        at org.opensearch.test.OpenSearchTestCase.after(OpenSearchTestCase.java:428)

  1>     test: index sort..........FAILED
  1>     WARNING: exorciseIndex() would remove reference to this segment; full exception:
  1> java.lang.IllegalArgumentException: Field rank is indexed with 4 bytes per dimension, but org.apache.lucene.search.SortedNumericSortField$3@13ca0f8e expected 8
  1> 	at org.apache.lucene.search.comparators.NumericComparator$NumericLeafComparator.<init>(NumericComparator.java:116)

This is caused by Lucene 9 upgrade, and is tracked in issue #2063 (comment)

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-17T00:00:12Z

❌ Gradle Check failure 466b083
Log 3461

Reports 3461

opensearch-ci-bot · 2022-03-17T00:00:21Z

❌ Gradle Check failure a17afa7
Log 3462

Reports 3462

opensearch-ci-bot · 2022-03-17T00:00:22Z

❌ Gradle Check failure c46e535
Log 3463

Reports 3463

Signed-off-by: Tianli Feng <ftianli@amazon.com>

owaiskazi19 · 2022-03-17T00:47:53Z

server/src/main/java/org/opensearch/rest/action/cat/RestMasterAction.java

@@ -50,18 +50,19 @@
 public class RestMasterAction extends AbstractCatAction {


Such class name won't be considered as inclusive?

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

Sounds good. Thanks for clearing it.

You are welcome! 😁

opensearch-ci-bot · 2022-03-17T01:13:16Z

✅ Gradle Check success 0b7b236
Log 3467

Reports 3467

Signed-off-by: Tianli Feng <ftianli@amazon.com>

opensearch-ci-bot · 2022-03-17T06:26:43Z

✅ Gradle Check success 300f18b
Log 3471

Reports 3471

Tianli Feng added 2 commits March 2, 2022 21:37

Add _cat cluster_manager as the ReplacedRoute for _cat master rest api

a0c43a3

Signed-off-by: Tianli Feng <ftianli@amazon.com>

Add yaml rest test for cat cluster manager api

b23c566

Signed-off-by: Tianli Feng <ftianli@amazon.com>

tlfeng mentioned this pull request Mar 8, 2022

Add alternative usages for the REST APIs and configurations that contain "master" terminology #1549

Closed

12 tasks

tlfeng changed the title ~~Add a new REST API GET _cat/cluster_manager as the replacement of GET _cat/master~~ Add a new REST API 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' Mar 8, 2022

Remove an unused import

a33d574

Signed-off-by: Tianli Feng <ftianli@amazon.com>

tlfeng added v2.0.0 Version 2.0.0 enhancement Enhancement or improvement to existing feature or request labels Mar 8, 2022

saratvemulapalli reviewed Mar 8, 2022

View reviewed changes

Align the text in yml

bb79c85

Signed-off-by: Tianli Feng <ftianli@amazon.com>

Add a comment to indicate the removal in the future.

c6fe237

Signed-off-by: Tianli Feng <ftianli@amazon.com>

tlfeng marked this pull request as ready for review March 9, 2022 01:09

tlfeng requested a review from a team as a code owner March 9, 2022 01:09

Deprecate path cat master in rest api spec

5a751e6

Signed-off-by: Tianli Feng <ftianli@amazon.com>

kartg reviewed Mar 16, 2022

View reviewed changes

Tianli Feng added 2 commits March 15, 2022 22:51

Merge remote-tracking branch 'upstream/main' into replace-cat-master-api

c9c1a0b

Signed-off-by: Tianli Feng <ftianli@amazon.com>

Rename master to cluster-manager in cat.cluster_manager.json

be44736

Signed-off-by: Tianli Feng <ftianli@amazon.com>

Tianli Feng added 4 commits March 16, 2022 16:07

Merge branch 'main' into replace-cat-master-api

00a6ff6

Signed-off-by: Tianli Feng <ftianli@amazon.com>

Remove the newly created cat.cluster_manager.json

466b083

Signed-off-by: Tianli Feng <ftianli@amazon.com>

Remove the test for newly created cat.cluster_manager.json

a17afa7

Signed-off-by: Tianli Feng <ftianli@amazon.com>

Rename cat.master rest-api-spec to cat.cluster_manager

c46e535

Signed-off-by: Tianli Feng <ftianli@amazon.com>

tlfeng changed the title ~~Add a new REST API 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master'~~ Add a new REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' Mar 16, 2022

tlfeng changed the title ~~Add a new REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master'~~ Add a REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' Mar 16, 2022

Fix yaml rest test by remove cat.master

0b7b236

Signed-off-by: Tianli Feng <ftianli@amazon.com>

tlfeng changed the title ~~Add a REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master'~~ Add a new REST API endpoint 'GET _cat/cluster_manager' as the replacement of 'GET _cat/master' Mar 17, 2022

owaiskazi19 reviewed Mar 17, 2022

View reviewed changes

put allowed_warnings in setup section

300f18b

Signed-off-by: Tianli Feng <ftianli@amazon.com>

kartg approved these changes Mar 17, 2022

View reviewed changes

tlfeng merged commit 3da9eb6 into opensearch-project:main Mar 18, 2022

tlfeng deleted the replace-cat-master-api branch March 18, 2022 22:00

tlfeng mentioned this pull request Apr 5, 2022

Test failure v2.0.0#mixedClusterTest o.o.backwards.ExceptionIT caused by 'GET _cat/master' deprecation warning #2759

Closed

		@@ -50,18 +50,19 @@
		public class RestMasterAction extends AbstractCatAction {

Conversation

tlfeng commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

opensearch-ci-bot commented Mar 8, 2022

Uh oh!

opensearch-ci-bot commented Mar 8, 2022

Uh oh!

opensearch-ci-bot commented Mar 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Mar 8, 2022

Uh oh!

opensearch-ci-bot commented Mar 9, 2022

Uh oh!

opensearch-ci-bot commented Mar 11, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlfeng Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlfeng Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlfeng Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Mar 16, 2022

Uh oh!

tlfeng commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opensearch-ci-bot commented Mar 17, 2022

Uh oh!

opensearch-ci-bot commented Mar 17, 2022

Uh oh!

opensearch-ci-bot commented Mar 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Mar 17, 2022

Uh oh!

opensearch-ci-bot commented Mar 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tlfeng commented Mar 8, 2022 •

edited

Loading

tlfeng Mar 16, 2022 •

edited

Loading

tlfeng Mar 16, 2022 •

edited

Loading

tlfeng Mar 17, 2022 •

edited

Loading

tlfeng commented Mar 16, 2022 •

edited

Loading