From a87c9d4455f042cf14e684c7f4dce1904dd0dbe5 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Fri, 18 Mar 2022 15:02:06 -0700 Subject: [PATCH] Add request parameter 'cluster_manager_timeout' as the alternative for '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 --- .../rest-api-spec/api/cat.nodes.json | 10 ++++- .../java/org/opensearch/rest/RestRequest.java | 27 ++++++++++++++ .../rest/action/cat/RestNodesAction.java | 21 ++++++++++- .../org/opensearch/rest/RestRequestTests.java | 37 +++++++++++++++++++ .../rest/action/cat/RestNodesActionTests.java | 18 +++++++++ 5 files changed, 111 insertions(+), 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 index ba3faa92c5ec6..abb13d015b4d5 100644 --- 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 index 7d11da7e122cd..e04d8faa8af39 100644 --- 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 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 index bce9b2d6b7e9d..7899909cdebbf 100644 --- 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 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(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 index 7abc53e4ca610..d5a915b42cf87 100644 --- 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 params) { Map> 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 index 593ad2907797e..9293d40605f42 100644 --- 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); + } }