Skip to content

Commit dcfa3fb

Browse files
Centralize codes related to 'master_timeout' deprecation for eaiser removal - 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 <ftianli@amazon.com>
1 parent aa8fbae commit dcfa3fb

File tree

6 files changed

+145
-101
lines changed

6 files changed

+145
-101
lines changed

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

+31
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,12 @@
3636
import org.apache.logging.log4j.Logger;
3737
import org.apache.lucene.search.spell.LevenshteinDistance;
3838
import org.apache.lucene.util.CollectionUtil;
39+
import org.opensearch.OpenSearchParseException;
40+
import org.opensearch.action.support.master.MasterNodeRequest;
3941
import org.opensearch.client.node.NodeClient;
4042
import org.opensearch.common.CheckedConsumer;
4143
import org.opensearch.common.collect.Tuple;
44+
import org.opensearch.common.logging.DeprecationLogger;
4245
import org.opensearch.common.settings.Setting;
4346
import org.opensearch.common.settings.Setting.Property;
4447
import org.opensearch.plugins.ActionPlugin;
@@ -200,6 +203,34 @@ protected Set<String> responseParams() {
200203
return Collections.emptySet();
201204
}
202205

206+
/**
207+
* Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used.
208+
* It also validates whether the two parameters 'master_timeout' and 'cluster_manager_timeout' are not assigned together.
209+
* The method is temporarily added in 2.0 duing applying inclusive language. Remove the method along with MASTER_ROLE.
210+
* @param mnr the action request
211+
* @param request the REST request to handle
212+
* @param logger the logger that logs deprecation notices
213+
* @param logMsgKeyPrefix the key prefix of a deprecation message to avoid duplicate messages.
214+
*/
215+
public static void parseDeprecatedMasterTimeoutParameter(
216+
MasterNodeRequest mnr,
217+
RestRequest request,
218+
DeprecationLogger logger,
219+
String logMsgKeyPrefix
220+
) {
221+
final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
222+
"Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.";
223+
final String DUPLICATE_PARAMETER_ERROR_MESSAGE =
224+
"Please only use one of the request parameters [master_timeout, cluster_manager_timeout].";
225+
if (request.hasParam("master_timeout")) {
226+
logger.deprecate(logMsgKeyPrefix + "_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE);
227+
if (request.hasParam("cluster_manager_timeout")) {
228+
throw new OpenSearchParseException(DUPLICATE_PARAMETER_ERROR_MESSAGE);
229+
}
230+
mnr.masterNodeTimeout(request.paramAsTime("master_timeout", mnr.masterNodeTimeout()));
231+
}
232+
}
233+
203234
public static class Wrapper extends BaseRestHandler {
204235

205236
protected final BaseRestHandler delegate;

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

-27
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@
5454
import java.io.IOException;
5555
import java.io.InputStream;
5656
import java.util.ArrayList;
57-
import java.util.Arrays;
5857
import java.util.Collections;
5958
import java.util.HashMap;
6059
import java.util.HashSet;
@@ -579,32 +578,6 @@ public static XContentType parseContentType(List<String> header) {
579578
throw new IllegalArgumentException("empty Content-Type header");
580579
}
581580

582-
/**
583-
* 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.
584-
* If the 2 values are not the same, throw an {@link OpenSearchParseException}.
585-
* @param keys Names of the request parameters.
586-
* @deprecated The method will be removed along with the request parameter "master_timeout".
587-
*/
588-
@Deprecated
589-
public void validateParamValuesAreEqual(String... keys) {
590-
// Track the last seen value and ensure that every subsequent value matches it.
591-
// The value to be tracked is the non-empty values of the parameters with the key.
592-
String lastSeenValue = null;
593-
for (String key : keys) {
594-
String value = param(key);
595-
if (!Strings.isNullOrEmpty(value)) {
596-
if (lastSeenValue == null || value.equals(lastSeenValue)) {
597-
lastSeenValue = value;
598-
} else {
599-
throw new OpenSearchParseException(
600-
"The values of the request parameters: {} are required to be equal, otherwise please only assign value to one of the request parameters.",
601-
Arrays.toString(keys)
602-
);
603-
}
604-
}
605-
}
606-
}
607-
608581
public static class ContentTypeHeaderException extends RuntimeException {
609582

610583
ContentTypeHeaderException(final IllegalArgumentException cause) {

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

+1-19
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,6 @@ public class RestNodesAction extends AbstractCatAction {
8686
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestNodesAction.class);
8787
static final String LOCAL_DEPRECATED_MESSAGE = "Deprecated parameter [local] used. This parameter does not cause this API to act "
8888
+ "locally, and should not be used. It will be unsupported in version 8.0.";
89-
static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
90-
"Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.";
9189

9290
@Override
9391
public List<Route> routes() {
@@ -113,7 +111,7 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli
113111
}
114112
clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local()));
115113
clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout()));
116-
parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request);
114+
parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request, deprecationLogger, getName());
117115
final boolean fullId = request.paramAsBoolean("full_id", false);
118116
return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<ClusterStateResponse>(channel) {
119117
@Override
@@ -529,20 +527,4 @@ Table buildTable(
529527
private short calculatePercentage(long used, long max) {
530528
return max <= 0 ? 0 : (short) ((100d * used) / max);
531529
}
532-
533-
/**
534-
* Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used.
535-
* It also validates whether the value of 'master_timeout' is the same with 'cluster_manager_timeout'.
536-
* Remove the method along with MASTER_ROLE.
537-
* @deprecated As of 2.0, because promoting inclusive language.
538-
*/
539-
@Deprecated
540-
private void parseDeprecatedMasterTimeoutParameter(ClusterStateRequest clusterStateRequest, RestRequest request) {
541-
final String deprecatedTimeoutParam = "master_timeout";
542-
if (request.hasParam(deprecatedTimeoutParam)) {
543-
deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE);
544-
request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout");
545-
clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout()));
546-
}
547-
}
548530
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
9+
package org.opensearch.action;
10+
11+
import org.junit.After;
12+
import org.opensearch.OpenSearchParseException;
13+
import org.opensearch.action.support.master.MasterNodeRequest;
14+
import org.opensearch.client.node.NodeClient;
15+
import org.opensearch.common.logging.DeprecationLogger;
16+
import org.opensearch.common.settings.Settings;
17+
import org.opensearch.rest.BaseRestHandler;
18+
import org.opensearch.rest.action.cat.RestNodesAction;
19+
import org.opensearch.test.OpenSearchTestCase;
20+
import org.opensearch.test.rest.FakeRestRequest;
21+
import org.opensearch.threadpool.TestThreadPool;
22+
23+
import static org.hamcrest.Matchers.containsString;
24+
25+
/**
26+
* As of 2.0, the request parameter 'master_timeout' in all applicable REST APIs is deprecated,
27+
* and alternative parameter 'cluster_manager_timeout' is added.
28+
* The tests are used to validate the behavior about the renamed request parameter.
29+
* Remove the test after removing MASTER_ROLE and 'master_timeout'.
30+
*/
31+
public class RenamedTimeoutRequestParameterTests extends OpenSearchTestCase {
32+
private final TestThreadPool threadPool = new TestThreadPool(RenamedTimeoutRequestParameterTests.class.getName());
33+
private final NodeClient client = new NodeClient(Settings.EMPTY, threadPool);
34+
private final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RenamedTimeoutRequestParameterTests.class);
35+
36+
private static final String DUPLICATE_PARAMETER_ERROR_MESSAGE =
37+
"Please only use one of the request parameters [master_timeout, cluster_manager_timeout].";
38+
private static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
39+
"Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.";
40+
41+
@After
42+
public void terminateThreadPool() {
43+
terminate(threadPool);
44+
}
45+
46+
public void testNoWarningsForNewParam() {
47+
BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
48+
getMasterNodeRequest(),
49+
getRestRequestWithNewParam(),
50+
deprecationLogger,
51+
"test"
52+
);
53+
}
54+
55+
public void testDeprecationWarningForOldParam() {
56+
BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
57+
getMasterNodeRequest(),
58+
getRestRequestWithDeprecatedParam(),
59+
deprecationLogger,
60+
"test"
61+
);
62+
assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
63+
}
64+
65+
public void testBothParamsNotValid() {
66+
Exception e = assertThrows(
67+
OpenSearchParseException.class,
68+
() -> BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
69+
getMasterNodeRequest(),
70+
getRestRequestWithBothParams(),
71+
deprecationLogger,
72+
"test"
73+
)
74+
);
75+
assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
76+
assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
77+
}
78+
79+
public void testCatAllocation() {
80+
RestNodesAction action = new RestNodesAction();
81+
Exception e = assertThrows(OpenSearchParseException.class, () -> action.doCatRequest(getRestRequestWithBothParams(), client));
82+
assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
83+
assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
84+
}
85+
86+
private MasterNodeRequest getMasterNodeRequest() {
87+
return new MasterNodeRequest() {
88+
@Override
89+
public ActionRequestValidationException validate() {
90+
return null;
91+
}
92+
};
93+
}
94+
95+
private FakeRestRequest getRestRequestWithBothParams() {
96+
FakeRestRequest request = new FakeRestRequest();
97+
request.params().put("cluster_manager_timeout", "1h");
98+
request.params().put("master_timeout", "3s");
99+
return request;
100+
}
101+
102+
private FakeRestRequest getRestRequestWithDeprecatedParam() {
103+
FakeRestRequest request = new FakeRestRequest();
104+
request.params().put("master_timeout", "3s");
105+
return request;
106+
}
107+
108+
private FakeRestRequest getRestRequestWithNewParam() {
109+
FakeRestRequest request = new FakeRestRequest();
110+
request.params().put("cluster_manager_timeout", "2m");
111+
return request;
112+
}
113+
}

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

-37
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434

3535
import org.opensearch.OpenSearchParseException;
3636
import org.opensearch.common.CheckedConsumer;
37-
import org.opensearch.common.Strings;
3837
import org.opensearch.common.bytes.BytesArray;
3938
import org.opensearch.common.bytes.BytesReference;
4039
import org.opensearch.common.collect.MapBuilder;
@@ -51,13 +50,11 @@
5150
import java.util.Collections;
5251
import java.util.HashMap;
5352
import java.util.List;
54-
import java.util.Locale;
5553
import java.util.Map;
5654
import java.util.concurrent.atomic.AtomicReference;
5755

5856
import static java.util.Collections.emptyMap;
5957
import static java.util.Collections.singletonMap;
60-
import static org.hamcrest.CoreMatchers.containsString;
6158
import static org.hamcrest.Matchers.equalTo;
6259
import static org.hamcrest.Matchers.instanceOf;
6360
import static org.mockito.Mockito.mock;
@@ -283,40 +280,6 @@ public void testRequiredContent() {
283280
assertEquals("unknown content type", e.getMessage());
284281
}
285282

286-
/*
287-
* The test is added in 2.0 when the request parameter "cluster_manager_timeout" is introduced.
288-
* Remove the test along with the removal of the non-inclusive terminology "master_timeout".
289-
*/
290-
public void testValidateParamValuesAreEqualWhenTheyAreEqual() {
291-
FakeRestRequest request = new FakeRestRequest();
292-
String valueForKey1 = randomFrom("value1", "", null);
293-
String valueForKey2 = "value1";
294-
request.params().put("key1", valueForKey1);
295-
request.params().put("key2", valueForKey2);
296-
request.validateParamValuesAreEqual("key1", "key2");
297-
assertTrue(
298-
String.format(
299-
Locale.ROOT,
300-
"The 2 values should be equal, or having 1 null/empty value. Value of key1: %s. Value of key2: %s",
301-
valueForKey1,
302-
valueForKey2
303-
),
304-
Strings.isNullOrEmpty(valueForKey1) || valueForKey1.equals(valueForKey2)
305-
);
306-
}
307-
308-
/*
309-
* The test is added in 2.0 when the request parameter "cluster_manager_timeout" is introduced.
310-
* Remove the test along with the removal of the non-inclusive terminology "master_timeout".
311-
*/
312-
public void testValidateParamValuesAreEqualWhenTheyAreNotEqual() {
313-
FakeRestRequest request = new FakeRestRequest();
314-
request.params().put("key1", "value1");
315-
request.params().put("key2", "value2");
316-
Exception e = assertThrows(OpenSearchParseException.class, () -> request.validateParamValuesAreEqual("key1", "key2"));
317-
assertThat(e.getMessage(), containsString("The values of the request parameters: [key1, key2] are required to be equal"));
318-
}
319-
320283
private static RestRequest contentRestRequest(String content, Map<String, String> params) {
321284
Map<String, List<String>> headers = new HashMap<>();
322285
headers.put("Content-Type", Collections.singletonList("application/json"));

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

-18
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232

3333
package org.opensearch.rest.action.cat;
3434

35-
import org.opensearch.OpenSearchParseException;
3635
import org.opensearch.Version;
3736
import org.opensearch.action.admin.cluster.node.info.NodesInfoResponse;
3837
import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse;
@@ -52,7 +51,6 @@
5251

5352
import static java.util.Collections.emptyMap;
5453
import static java.util.Collections.emptySet;
55-
import static org.hamcrest.CoreMatchers.containsString;
5654
import static org.mockito.Mockito.mock;
5755
import static org.mockito.Mockito.when;
5856

@@ -91,20 +89,4 @@ public void testCatNodesWithLocalDeprecationWarning() {
9189

9290
terminate(threadPool);
9391
}
94-
95-
/**
96-
* Validate both cluster_manager_timeout and its predecessor can be parsed correctly.
97-
* Remove the test along with MASTER_ROLE. It's added in version 2.0.0.
98-
*/
99-
public void testCatNodesWithClusterManagerTimeout() {
100-
TestThreadPool threadPool = new TestThreadPool(RestNodesActionTests.class.getName());
101-
NodeClient client = new NodeClient(Settings.EMPTY, threadPool);
102-
FakeRestRequest request = new FakeRestRequest();
103-
request.params().put("cluster_manager_timeout", randomFrom("1h", "2m"));
104-
request.params().put("master_timeout", "3s");
105-
Exception e = assertThrows(OpenSearchParseException.class, () -> action.doCatRequest(request, client));
106-
assertThat(e.getMessage(), containsString("[master_timeout, cluster_manager_timeout] are required to be equal"));
107-
assertWarnings(RestNodesAction.MASTER_TIMEOUT_DEPRECATED_MESSAGE);
108-
terminate(threadPool);
109-
}
11092
}

0 commit comments

Comments
 (0)