Skip to content

Commit

Permalink
Centralize codes related to 'master_timeout' deprecation for eaiser r…
Browse files Browse the repository at this point in the history
…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]>
  • Loading branch information
opensearch-trigger-bot[bot] authored Apr 1, 2022
1 parent aa8fbae commit dcfa3fb
Show file tree
Hide file tree
Showing 6 changed files with 145 additions and 101 deletions.
31 changes: 31 additions & 0 deletions server/src/main/java/org/opensearch/rest/BaseRestHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,12 @@
import org.apache.logging.log4j.Logger;
import org.apache.lucene.search.spell.LevenshteinDistance;
import org.apache.lucene.util.CollectionUtil;
import org.opensearch.OpenSearchParseException;
import org.opensearch.action.support.master.MasterNodeRequest;
import org.opensearch.client.node.NodeClient;
import org.opensearch.common.CheckedConsumer;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Setting;
import org.opensearch.common.settings.Setting.Property;
import org.opensearch.plugins.ActionPlugin;
Expand Down Expand Up @@ -200,6 +203,34 @@ protected Set<String> responseParams() {
return Collections.emptySet();
}

/**
* Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used.
* It also validates whether the two parameters 'master_timeout' and 'cluster_manager_timeout' are not assigned together.
* The method is temporarily added in 2.0 duing applying inclusive language. Remove the method along with MASTER_ROLE.
* @param mnr the action request
* @param request the REST request to handle
* @param logger the logger that logs deprecation notices
* @param logMsgKeyPrefix the key prefix of a deprecation message to avoid duplicate messages.
*/
public static void parseDeprecatedMasterTimeoutParameter(
MasterNodeRequest mnr,
RestRequest request,
DeprecationLogger logger,
String logMsgKeyPrefix
) {
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.";
final String DUPLICATE_PARAMETER_ERROR_MESSAGE =
"Please only use one of the request parameters [master_timeout, cluster_manager_timeout].";
if (request.hasParam("master_timeout")) {
logger.deprecate(logMsgKeyPrefix + "_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE);
if (request.hasParam("cluster_manager_timeout")) {
throw new OpenSearchParseException(DUPLICATE_PARAMETER_ERROR_MESSAGE);
}
mnr.masterNodeTimeout(request.paramAsTime("master_timeout", mnr.masterNodeTimeout()));
}
}

public static class Wrapper extends BaseRestHandler {

protected final BaseRestHandler delegate;
Expand Down
27 changes: 0 additions & 27 deletions server/src/main/java/org/opensearch/rest/RestRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
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;
Expand Down Expand Up @@ -579,32 +578,6 @@ 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,6 @@ 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() {
Expand All @@ -113,7 +111,7 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli
}
clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local()));
clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout()));
parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request);
parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request, deprecationLogger, getName());
final boolean fullId = request.paramAsBoolean("full_id", false);
return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<ClusterStateResponse>(channel) {
@Override
Expand Down Expand Up @@ -529,20 +527,4 @@ 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()));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.action;

import org.junit.After;
import org.opensearch.OpenSearchParseException;
import org.opensearch.action.support.master.MasterNodeRequest;
import org.opensearch.client.node.NodeClient;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Settings;
import org.opensearch.rest.BaseRestHandler;
import org.opensearch.rest.action.cat.RestNodesAction;
import org.opensearch.test.OpenSearchTestCase;
import org.opensearch.test.rest.FakeRestRequest;
import org.opensearch.threadpool.TestThreadPool;

import static org.hamcrest.Matchers.containsString;

/**
* As of 2.0, the request parameter 'master_timeout' in all applicable REST APIs is deprecated,
* and alternative parameter 'cluster_manager_timeout' is added.
* The tests are used to validate the behavior about the renamed request parameter.
* Remove the test after removing MASTER_ROLE and 'master_timeout'.
*/
public class RenamedTimeoutRequestParameterTests extends OpenSearchTestCase {
private final TestThreadPool threadPool = new TestThreadPool(RenamedTimeoutRequestParameterTests.class.getName());
private final NodeClient client = new NodeClient(Settings.EMPTY, threadPool);
private final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RenamedTimeoutRequestParameterTests.class);

private static final String DUPLICATE_PARAMETER_ERROR_MESSAGE =
"Please only use one of the request parameters [master_timeout, cluster_manager_timeout].";
private 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.";

@After
public void terminateThreadPool() {
terminate(threadPool);
}

public void testNoWarningsForNewParam() {
BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
getMasterNodeRequest(),
getRestRequestWithNewParam(),
deprecationLogger,
"test"
);
}

public void testDeprecationWarningForOldParam() {
BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
getMasterNodeRequest(),
getRestRequestWithDeprecatedParam(),
deprecationLogger,
"test"
);
assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
}

public void testBothParamsNotValid() {
Exception e = assertThrows(
OpenSearchParseException.class,
() -> BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
getMasterNodeRequest(),
getRestRequestWithBothParams(),
deprecationLogger,
"test"
)
);
assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
}

public void testCatAllocation() {
RestNodesAction action = new RestNodesAction();
Exception e = assertThrows(OpenSearchParseException.class, () -> action.doCatRequest(getRestRequestWithBothParams(), client));
assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
}

private MasterNodeRequest getMasterNodeRequest() {
return new MasterNodeRequest() {
@Override
public ActionRequestValidationException validate() {
return null;
}
};
}

private FakeRestRequest getRestRequestWithBothParams() {
FakeRestRequest request = new FakeRestRequest();
request.params().put("cluster_manager_timeout", "1h");
request.params().put("master_timeout", "3s");
return request;
}

private FakeRestRequest getRestRequestWithDeprecatedParam() {
FakeRestRequest request = new FakeRestRequest();
request.params().put("master_timeout", "3s");
return request;
}

private FakeRestRequest getRestRequestWithNewParam() {
FakeRestRequest request = new FakeRestRequest();
request.params().put("cluster_manager_timeout", "2m");
return request;
}
}
37 changes: 0 additions & 37 deletions server/src/test/java/org/opensearch/rest/RestRequestTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@

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;
Expand All @@ -51,13 +50,11 @@
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;
Expand Down Expand Up @@ -283,40 +280,6 @@ 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"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@

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;
Expand All @@ -52,7 +51,6 @@

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;

Expand Down Expand Up @@ -91,20 +89,4 @@ 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);
}
}

0 comments on commit dcfa3fb

Please sign in to comment.