Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT Nodes API #2435

tlfeng · 2022-03-11T00:53:11Z

Description

master_timeout is being used in multiple APIs, such as cat allocation and cat nodes APIs.
The PR take CAT Nodes API as a sample.

Add a new request parameter cluster_manager_timeout, as the alternative for the existing master_timeout, in CAT Nodes API
Throw an exception when both master_timeout and cluster_manager_timeout are assigned but with different values, such as GET _cat/nodes?v&master_timeout=1s&cluster_manager_timeout=2s. This is addressed by the temporary method validateParamValuesAreEqual() added in RestRequest class.
(The above behavior is changed by commit e1ee222 / PR Centralize codes related to 'master_timeout' deprecation for eaiser removal - in CAT Nodes API #2670)
Throw an exception when both master_timeout and cluster_manager_timeout are assigned, no matter the assigned value are same or not, such as GET _cat/nodes?v&master_timeout=1s&cluster_manager_timeout=1s.
Add deprecation log when using request parameter master_timeout
Deprecate request parameter master_timeout of in rest api spec
Add unit tests

Deprecation notice:
With the built-in deprecation logger of OpenSearch (code), the following message will be recorded in the log files:
runTask_deprecation.log

[2022-03-14T20:32:37,588][DEPRECATION][o.o.d.r.a.c.RestNodesAction] [runTask-0] Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.

runTask_deprecation.json

{"type": "deprecation", "timestamp": "2022-03-14T20:32:37,588-07:00", "level": "DEPRECATION", "component": "o.o.d.r.a.c.RestNodesAction", "cluster.name": "runTask", "node.name": "runTask-0", "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.", "cluster.uuid": "rvONWP9qT8uj7_oYihbu4A", "node.id": "sxmmw2FbSG6vCXpPv4rpuQ"  }

Testing:
When the values of old and new parameters are not the same:

GET http://localhost:9200/_cat/nodes?v&master_timeout=1s&cluster_manager_timeout=2s

{
    "error": {
        "root_cause": [
            {
                "type": "parse_exception",
                "reason": "The values of the request parameters: [master_timeout, cluster_manager_timeout] are required to be equal, otherwise please only assign value to one of the request parameters."
            }
        ],
        "type": "parse_exception",
        "reason": "The values of the request parameters: [master_timeout, cluster_manager_timeout] are required to be equal, otherwise please only assign value to one of the request parameters."
    },
    "status": 400
}

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.

opensearch-ci-bot · 2022-03-11T00:54:02Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2022-03-11T01:18:54Z

❌ Gradle Check failure b842e508de00a4009e250409fa0bcaabffbba79a
Log 3240

Reports 3240

opensearch-ci-bot · 2022-03-11T01:46:35Z

❌ Gradle Check failure e16ce49116bb24ba1f94364a576dde27367a114b
Log 3242

Reports 3242

opensearch-ci-bot · 2022-03-11T01:46:44Z

❌ Gradle Check failure 009ded2aed9f96d8a157cbe74bbf6edabf1407bb
Log 3243

Reports 3243

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-11T03:03:05Z

✅ Gradle Check success 24cf061
Log 3246

Reports 3246

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-11T07:19:58Z

❌ Gradle Check failure 67d9e1b
Log 3259

Reports 3259

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-13T07:45:19Z

❌ Gradle Check failure 3f46f4e
Log 3310

Reports 3310

…-timeout Signed-off-by: Tianli Feng <[email protected]>

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-15T00:50:30Z

❌ Gradle Check failure db4ba0f9afafec513be70e53d6c14a754212f7c5
Log 3371

Reports 3371

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-15T01:12:28Z

✅ Gradle Check success 8e0e074
Log 3372

Reports 3372

opensearch-ci-bot · 2022-03-15T01:41:30Z

❌ Gradle Check failure dec2597
Log 3377

Reports 3377

tlfeng · 2022-03-15T01:46:09Z

in log 3377

> Task :qa:mixed-cluster:v1.2.5#mixedClusterTest

REPRODUCE WITH: ./gradlew ':qa:mixed-cluster:v1.2.5#mixedClusterTest' --tests "org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT" -Dtests.method="test {p0=indices.get_field_mapping/20_missing_field/Return empty object if field doesn't exist, but index does}" -Dtests.seed=E592FBD43C377701 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=fr-LU -Dtests.timezone=Australia/Tasmania -Druntime.java=17

org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT > test {p0=indices.get_field_mapping/20_missing_field/Return empty object if field doesn't exist, but index does} FAILED
    java.lang.AssertionError: Failure at [indices.get_field_mapping/20_missing_field:20]: field [test_index.mappings] is null
        at __randomizedtesting.SeedInfo.seed([E592FBD43C377701:6DC6C40E92CB1AF9]:0)
        at org.opensearch.test.rest.yaml.OpenSearchClientYamlSuiteTestCase.executeSection(OpenSearchClientYamlSuiteTestCase.java:442)
        at org.opensearch.test.rest.yaml.OpenSearchClientYamlSuiteTestCase.test(OpenSearchClientYamlSuiteTestCase.java:415)

It's reported in issue #2440

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-15T07:02:17Z

❌ Gradle Check failure 2d36c8f
Log 3390

Reports 3390

kartg · 2022-03-16T00:37:54Z

server/src/main/java/org/opensearch/rest/RestRequest.java

+    @Deprecated
+    public void validateParamValuesAreEqual(String... keys) {
+        // Filter the non-empty values of the parameters with the key, and get the distinct values.
+        Set<String> set = new HashSet<>();


Minor improvement - you don't need a Set to check this. You can just track the last seen value and ensure that every subsequent value matches it. This would also force a break out of the loop as soon as there is a mismatch rather than attempting to parse all values of the input keys.

Hi @kartg Thank you so much for your careful review and your valuable opinion!
I will make the changes according to your suggestion!

Updated in the commit 02e68b5. I learnt the nice way to check if all the values are the same, thank you! 👍

kartg · 2022-03-16T00:44:19Z

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

+        if (request.hasParam(deprecatedTimeoutParam)) {
+            deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+            request.validateParamValuesAreEqual(deprecatedTimeoutParam, clusterManagerTimeoutParam);
+        }


Two nitpicks here:

Setting the masterNodeTimeout should occur within the if-condition

The validation could occur before setting the value

Great suggestion! Now the procedure is more reasonable 😄. I modified the code in the commit 399f2d9

kartg · 2022-03-16T00:55:37Z

server/src/test/java/org/opensearch/rest/RestRequestTests.java

+                    + valueForKey2,
+                Strings.isNullOrEmpty(valueForKey1) || Strings.isNullOrEmpty(valueForKey2) || valueForKey1.equals(valueForKey2)
+            );
+        } catch (OpenSearchParseException e) {


Can we please split this into two tests?

We should deterministically test both cases - when the two are equal and for inequality - for all test runs rather than testing either code path through a random.

Thanks for telling me the opinion to design test case! Updated along wit the above comment 02e68b5

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-16T23:27:16Z

✅ Gradle Check success 399f2d9
Log 3459

Reports 3459

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-17T11:36:56Z

✅ Gradle Check success b99c8d5
Log 3482

Reports 3482

…-timeout Signed-off-by: Tianli Feng <[email protected]>

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-17T23:08:08Z

✅ Gradle Check success dfd260d
Log 3502

Reports 3502

…emoval - in CAT Nodes API (#2670) (#2695) The request parameter `master_timeout` for CAT Nodes API is deprecated and alternative parameter `cluster_manager_timeout` is added in commit a87c9d4 / PR #2435. This PR refactors a previous code commit, and it's suggested by #2658 (comment). Change: This PR put the temporary code related to the 'master_timeout' deprecation in centralized places, so that it will be easier to remove when removing the deprecated parameter `master_timeout` in the future. - Move the method `parseDeprecatedMasterTimeoutParameter()` into abstract base class `BaseRestHandler`, so that every REST API handler can call it. - Put the unit tests related to the 'master_timeout' deprecation in one class. Another notable change: Prohibit using two paramters `master_timeout` and `cluster_manager_timeout` together. Reason: There are other 60 REST APIs have got request parameter `master_timeout`, and the parameter pending to be deprecated. (See issue #2511 for the full list). - Adding new codes (creating deprecation warning, validating the parameter value and unit tests) in every class for the 60 APIs will cause large duplication. - Removing the duplicate deprecated codes in the future is also a trouble. Signed-off-by: Tianli Feng <[email protected]>

…emoval - in CAT Nodes API (#2670) (#2696) The request parameter `master_timeout` for CAT Nodes API is deprecated and alternative parameter `cluster_manager_timeout` is added in commit a87c9d4 / PR #2435. This PR refactors a previous code commit, and it's suggested by #2658 (comment). Change: This PR put the temporary code related to the 'master_timeout' deprecation in centralized places, so that it will be easier to remove when removing the deprecated parameter `master_timeout` in the future. - Move the method `parseDeprecatedMasterTimeoutParameter()` into abstract base class `BaseRestHandler`, so that every REST API handler can call it. - Put the unit tests related to the 'master_timeout' deprecation in one class. Another notable change: Prohibit using two paramters `master_timeout` and `cluster_manager_timeout` together. Reason: There are other 60 REST APIs have got request parameter `master_timeout`, and the parameter pending to be deprecated. (See issue #2511 for the full list). - Adding new codes (creating deprecation warning, validating the parameter value and unit tests) in every class for the 60 APIs will cause large duplication. - Removing the duplicate deprecated codes in the future is also a trouble. Signed-off-by: Tianli Feng <[email protected]>

…r 'master_timeout', and deprecate 'master_timeout' - in CAT APIs (#2557) Apply the change of CAT Nodes API in PR #2435 to other applicable CAT APIs. - Deprecate the request parameter `master_timeout` that used in many CAT APIs. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]>

…r 'master_timeout', and deprecate 'master_timeout' - in CAT APIs (#2557) Apply the change of CAT Nodes API in PR #2435 to other applicable CAT APIs. - Deprecate the request parameter `master_timeout` that used in many CAT APIs. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]> (cherry picked from commit 78465b4)

… alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT APIs (#2716) Apply the change of CAT Nodes API in PR #2435 to other applicable CAT APIs. - Deprecate the request parameter `master_timeout` that used in many CAT APIs. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]> (cherry picked from commit 78465b4)

… alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT APIs (#2717) Apply the change of CAT Nodes API in PR #2435 to other applicable CAT APIs. - Deprecate the request parameter `master_timeout` that used in many CAT APIs. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]> (cherry picked from commit 78465b4)

tlfeng mentioned this pull request Mar 11, 2022

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

Closed

12 tasks

tlfeng force-pushed the add-cluster-manager-timeout branch from b842e50 to e16ce49 Compare March 11, 2022 01:19

tlfeng added enhancement Enhancement or improvement to existing feature or request v2.0.0 Version 2.0.0 documentation pending Tracks issues which have PRs merged but documentation changes pending labels Mar 11, 2022

tlfeng force-pushed the add-cluster-manager-timeout branch from e16ce49 to 009ded2 Compare March 11, 2022 01:24

Add request parameter cluster_manager_timeout

24cf061

Signed-off-by: Tianli Feng <[email protected]>

tlfeng force-pushed the add-cluster-manager-timeout branch from 009ded2 to 24cf061 Compare March 11, 2022 02:17

Add deprecation notice to master_timeout

67d9e1b

Signed-off-by: Tianli Feng <[email protected]>

Tianli Feng added 3 commits March 12, 2022 22:39

Merge branch 'main' into add-cluster-manager-timeout

d1a8fb9

Signed-off-by: Tianli Feng <[email protected]>

Revert deprecating methods in MasterNodeRequest

8da36a9

Signed-off-by: Tianli Feng <[email protected]>

Add todo to remove master_timeout

3f46f4e

Signed-off-by: Tianli Feng <[email protected]>

Tianli Feng added 2 commits March 14, 2022 15:53

Merge remote-tracking branch 'upstream/main' into add-cluster-manager…

aa50d4f

…-timeout Signed-off-by: Tianli Feng <[email protected]>

Add unit test for the temporary method ValidateParamValuesAreEqual()

8e0e074

Signed-off-by: Tianli Feng <[email protected]>

tlfeng force-pushed the add-cluster-manager-timeout branch from db4ba0f to 8e0e074 Compare March 15, 2022 00:21

Put codes of parsing master_timeout into a method

dec2597

Signed-off-by: Tianli Feng <[email protected]>

tlfeng changed the title ~~Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout'~~ Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout' - In CAT Nodes API Mar 15, 2022

Use Strings.isNullorEmpty

7bf815c

Signed-off-by: Tianli Feng <[email protected]>

Fix unit test by moving assert into try

2d36c8f

Signed-off-by: Tianli Feng <[email protected]>

dblock requested a review from andrross March 15, 2022 15:48

andrross removed their request for review March 15, 2022 19:15

kartg approved these changes Mar 16, 2022

View reviewed changes

Tianli Feng added 3 commits March 16, 2022 14:15

Merge branch 'main' into add-cluster-manager-timeout

7195d42

Signed-off-by: Tianli Feng <[email protected]>

Modify testValidateParamValuesAreEqual() and test

02e68b5

Signed-off-by: Tianli Feng <[email protected]>

Set masterNodeTimeout when the parameter master_timeout occurs

399f2d9

Signed-off-by: Tianli Feng <[email protected]>

tlfeng mentioned this pull request Mar 16, 2022

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

Merged

5 tasks

Tianli Feng added 2 commits March 17, 2022 03:34

Use assertThrows instead of try catch

ae7308c

Signed-off-by: Tianli Feng <[email protected]>

Merge branch 'main' into add-cluster-manager-timeout

b99c8d5

Signed-off-by: Tianli Feng <[email protected]>

Tianli Feng added 2 commits March 17, 2022 15:16

Merge remote-tracking branch 'upstream/main' into add-cluster-manager…

ec61416

…-timeout Signed-off-by: Tianli Feng <[email protected]>

put all values directly in RandomFrom in the unit test

dfd260d

Signed-off-by: Tianli Feng <[email protected]>

tlfeng mentioned this pull request Mar 18, 2022

Add a new request parameter "cluster_manager_timeout" in the REST APIs that accept "master_timeout" parameter, and deprecate "master_timeout" #2511

Closed

11 tasks

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

tlfeng deleted the add-cluster-manager-timeout branch March 18, 2022 22:02

tlfeng mentioned this pull request Mar 22, 2022

Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT APIs #2557

Merged

5 tasks

This was referenced Mar 29, 2022

Add request parameter 'cluster_manager_timeout' and deprecate 'master_timeout' - in Cluster APIs #2658

Merged

Centralize codes related to 'master_timeout' deprecation for eaiser removal - in CAT Nodes API #2670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT Nodes API #2435

Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT Nodes API #2435

tlfeng commented Mar 11, 2022 •

edited

Loading

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 13, 2022

opensearch-ci-bot commented Mar 15, 2022

opensearch-ci-bot commented Mar 15, 2022

opensearch-ci-bot commented Mar 15, 2022

tlfeng commented Mar 15, 2022

opensearch-ci-bot commented Mar 15, 2022

kartg Mar 16, 2022

tlfeng Mar 16, 2022 •

edited

Loading

tlfeng Mar 16, 2022

kartg Mar 16, 2022

tlfeng Mar 16, 2022

kartg Mar 16, 2022

tlfeng Mar 16, 2022

opensearch-ci-bot commented Mar 16, 2022

opensearch-ci-bot commented Mar 17, 2022

opensearch-ci-bot commented Mar 17, 2022

Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT Nodes API #2435

Add request parameter 'cluster_manager_timeout' as the alternative for 'master_timeout', and deprecate 'master_timeout' - in CAT Nodes API #2435

Conversation

tlfeng commented Mar 11, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 11, 2022

opensearch-ci-bot commented Mar 13, 2022

opensearch-ci-bot commented Mar 15, 2022

opensearch-ci-bot commented Mar 15, 2022

opensearch-ci-bot commented Mar 15, 2022

tlfeng commented Mar 15, 2022

opensearch-ci-bot commented Mar 15, 2022

kartg Mar 16, 2022

Choose a reason for hiding this comment

tlfeng Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

tlfeng Mar 16, 2022

Choose a reason for hiding this comment

kartg Mar 16, 2022

Choose a reason for hiding this comment

tlfeng Mar 16, 2022

Choose a reason for hiding this comment

kartg Mar 16, 2022

Choose a reason for hiding this comment

tlfeng Mar 16, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 16, 2022

opensearch-ci-bot commented Mar 17, 2022

opensearch-ci-bot commented Mar 17, 2022

tlfeng commented Mar 11, 2022 •

edited

Loading

tlfeng Mar 16, 2022 •

edited

Loading