honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 <ftianli@amazon.com>
This commit is contained in:
Tianli Feng 2022-03-18 15:02:06 -07:00 committed by GitHub
parent 3da9eb642c
commit a87c9d4455
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 111 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json
View File

@ -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",

27
server/src/main/java/org/opensearch/rest/RestRequest.java
View File

@ -54,6 +54,7 @@ import org.opensearch.http.HttpRequest;
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 class RestRequest implements ToXContent.Params {
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) {

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

@ -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 class RestNodesAction extends AbstractCatAction {
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 @@ public class RestNodesAction extends AbstractCatAction {
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()));
}
}
}

37
server/src/test/java/org/opensearch/rest/RestRequestTests.java
View File

@ -34,6 +34,7 @@ package org.opensearch.rest;
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.ArrayList;
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 class RestRequestTests extends OpenSearchTestCase {
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"));

18
server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java
View File

@ -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 java.util.Collections;
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 class RestNodesActionTests extends OpenSearchTestCase {
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);
}
}