Add request parameter 'cluster_manager_timeout' as the alternative fo…

…r 'master_timeout', and deprecate 'master_timeout' - in CAT Nodes API (#2435) * `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. * Add deprecation log when using request parameter `master_timeout` * Deprecate request parameter `master_timeout` of in rest api spec * Add unit tests Signed-off-by: Tianli Feng <[email protected]>
opensearch-project · Mar 18, 2022 · a87c9d4 · a87c9d4
1 parent 3da9eb6
commit a87c9d4
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 2 deletions.
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json
@@ -51,7 +51,15 @@
       },
       "master_timeout":{
         "type":"time",
-        "description":"Explicit operation timeout for connection to master node"
+        "description":"Explicit operation timeout for connection to master node",
+        "deprecated":{
+          "version":"2.0.0",
+          "description":"To promote inclusive language, use 'cluster_manager_timeout' instead."
+        }
+      },
+      "cluster_manager_timeout":{
+        "type":"time",
+        "description":"Explicit operation timeout for connection to cluster-manager node"
       },
       "h":{
         "type":"list",

diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java
@@ -54,6 +54,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -578,6 +579,32 @@ public static XContentType parseContentType(List<String> header) {
         throw new IllegalArgumentException("empty Content-Type header");
     }
 
+    /**
+     * The method is only used to validate whether the values of the 2 request parameters "master_timeout" and "cluster_manager_timeout" is the same value or not.
+     * If the 2 values are not the same, throw an {@link OpenSearchParseException}.
+     * @param keys Names of the request parameters.
+     * @deprecated The method will be removed along with the request parameter "master_timeout".
+     */
+    @Deprecated
+    public void validateParamValuesAreEqual(String... keys) {
+        // Track the last seen value and ensure that every subsequent value matches it.
+        // The value to be tracked is the non-empty values of the parameters with the key.
+        String lastSeenValue = null;
+        for (String key : keys) {
+            String value = param(key);
+            if (!Strings.isNullOrEmpty(value)) {
+                if (lastSeenValue == null || value.equals(lastSeenValue)) {
+                    lastSeenValue = value;
+                } else {
+                    throw new OpenSearchParseException(
+                        "The values of the request parameters: {} are required to be equal, otherwise please only assign value to one of the request parameters.",
+                        Arrays.toString(keys)
+                    );
+                }
+            }
+        }
+    }
+
     public static class ContentTypeHeaderException extends RuntimeException {
 
         ContentTypeHeaderException(final IllegalArgumentException cause) {

diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java
@@ -86,6 +86,8 @@ public class RestNodesAction extends AbstractCatAction {
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestNodesAction.class);
     static final String LOCAL_DEPRECATED_MESSAGE = "Deprecated parameter [local] used. This parameter does not cause this API to act "
         + "locally, and should not be used. It will be unsupported in version 8.0.";
+    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() {
@@ -110,7 +112,8 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli
             deprecationLogger.deprecate("cat_nodes_local_parameter", LOCAL_DEPRECATED_MESSAGE);
         }
         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);
         final boolean fullId = request.paramAsBoolean("full_id", false);
         return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<ClusterStateResponse>(channel) {
             @Override
@@ -525,4 +528,20 @@ Table buildTable(
     private short calculatePercentage(long used, long max) {
         return max <= 0 ? 0 : (short) ((100d * used) / max);
     }
+
+    /**
+     * 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 void parseDeprecatedMasterTimeoutParameter(ClusterStateRequest clusterStateRequest, RestRequest request) {
+        final String deprecatedTimeoutParam = "master_timeout";
+        if (request.hasParam(deprecatedTimeoutParam)) {
+            deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+            request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout");
+            clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout()));
+        }
+    }
 }
diff --git a/server/src/test/java/org/opensearch/rest/RestRequestTests.java b/server/src/test/java/org/opensearch/rest/RestRequestTests.java
@@ -34,6 +34,7 @@
 
 import org.opensearch.OpenSearchParseException;
 import org.opensearch.common.CheckedConsumer;
+import org.opensearch.common.Strings;
 import org.opensearch.common.bytes.BytesArray;
 import org.opensearch.common.bytes.BytesReference;
 import org.opensearch.common.collect.MapBuilder;
@@ -50,11 +51,13 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.mockito.Mockito.mock;
@@ -280,6 +283,40 @@ public void testRequiredContent() {
         assertEquals("unknown content type", e.getMessage());
     }
 
+    /*
+     * The test is added in 2.0 when the request parameter "cluster_manager_timeout" is introduced.
+     * Remove the test along with the removal of the non-inclusive terminology "master_timeout".
+     */
+    public void testValidateParamValuesAreEqualWhenTheyAreEqual() {
+        FakeRestRequest request = new FakeRestRequest();
+        String valueForKey1 = randomFrom("value1", "", null);
+        String valueForKey2 = "value1";
+        request.params().put("key1", valueForKey1);
+        request.params().put("key2", valueForKey2);
+        request.validateParamValuesAreEqual("key1", "key2");
+        assertTrue(
+            String.format(
+                Locale.ROOT,
+                "The 2 values should be equal, or having 1 null/empty value. Value of key1: %s. Value of key2: %s",
+                valueForKey1,
+                valueForKey2
+            ),
+            Strings.isNullOrEmpty(valueForKey1) || valueForKey1.equals(valueForKey2)
+        );
+    }
+
+    /*
+     * The test is added in 2.0 when the request parameter "cluster_manager_timeout" is introduced.
+     * Remove the test along with the removal of the non-inclusive terminology "master_timeout".
+     */
+    public void testValidateParamValuesAreEqualWhenTheyAreNotEqual() {
+        FakeRestRequest request = new FakeRestRequest();
+        request.params().put("key1", "value1");
+        request.params().put("key2", "value2");
+        Exception e = assertThrows(OpenSearchParseException.class, () -> request.validateParamValuesAreEqual("key1", "key2"));
+        assertThat(e.getMessage(), containsString("The values of the request parameters: [key1, key2] are required to be equal"));
+    }
+
     private static RestRequest contentRestRequest(String content, Map<String, String> params) {
         Map<String, List<String>> headers = new HashMap<>();
         headers.put("Content-Type", Collections.singletonList("application/json"));

diff --git a/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java b/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java
@@ -32,6 +32,7 @@
 
 package org.opensearch.rest.action.cat;
 
+import org.opensearch.OpenSearchParseException;
 import org.opensearch.Version;
 import org.opensearch.action.admin.cluster.node.info.NodesInfoResponse;
 import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse;
@@ -51,6 +52,7 @@
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.emptySet;
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -89,4 +91,20 @@ public void testCatNodesWithLocalDeprecationWarning() {
 
         terminate(threadPool);
     }
+
+    /**
+     * Validate both cluster_manager_timeout and its predecessor can be parsed correctly.
+     * Remove the test along with MASTER_ROLE. It's added in version 2.0.0.
+     */
+    public void testCatNodesWithClusterManagerTimeout() {
+        TestThreadPool threadPool = new TestThreadPool(RestNodesActionTests.class.getName());
+        NodeClient client = new NodeClient(Settings.EMPTY, threadPool);
+        FakeRestRequest request = new FakeRestRequest();
+        request.params().put("cluster_manager_timeout", randomFrom("1h", "2m"));
+        request.params().put("master_timeout", "3s");
+        Exception e = assertThrows(OpenSearchParseException.class, () -> action.doCatRequest(request, client));
+        assertThat(e.getMessage(), containsString("[master_timeout, cluster_manager_timeout] are required to be equal"));
+        assertWarnings(RestNodesAction.MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+        terminate(threadPool);
+    }
 }