Merge branch 'main' into manager-timeout-cluster-api

Signed-off-by: Tianli Feng <[email protected]> # Conflicts: # server/src/main/java/org/opensearch/rest/BaseRestHandler.java # server/src/test/java/org/opensearch/action/RenamedTimeoutRequestParameterTests.java
opensearch-project · Apr 1, 2022 · 1b383a1 · 1b383a1
2 parents 430c815 + e1ee222
commit 1b383a1
Show file tree

Hide file tree

Showing 11 changed files with 102 additions and 174 deletions.
diff --git a/.ci/bwcVersions b/.ci/bwcVersions
@@ -38,4 +38,5 @@ BWC_VERSION:
   - "1.2.5"
   - "1.3.0"
   - "1.3.1"
+  - "1.3.2"
   - "1.4.0"
diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar
diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties
@@ -11,7 +11,7 @@
 
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-7.4.1-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-7.4.2-all.zip
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionSha256Sum=a9a7b7baba105f6557c9dcf9c3c6e8f7e57e6b49889c5f1d133f015d0727e4be
+distributionSha256Sum=e6d864e3b5bc05cc62041842b306383fc1fefcec359e70cebb1d470a6094ca82
diff --git a/qa/wildfly/build.gradle b/qa/wildfly/build.gradle
@@ -40,7 +40,7 @@ testFixtures.useFixture()
 
 dependencies {
   providedCompile 'javax.enterprise:cdi-api:1.2'
-  providedCompile 'org.jboss.spec.javax.annotation:jboss-annotations-api_1.2_spec:1.0.0.Final'
+  providedCompile 'org.jboss.spec.javax.annotation:jboss-annotations-api_1.2_spec:1.0.2.Final'
   providedCompile 'org.jboss.spec.javax.ws.rs:jboss-jaxrs-api_2.0_spec:1.0.0.Final'
   api('org.jboss.resteasy:resteasy-jackson2-provider:3.0.19.Final') {
     exclude module: 'jackson-annotations'

diff --git a/server/src/main/java/org/opensearch/Version.java b/server/src/main/java/org/opensearch/Version.java
@@ -80,6 +80,7 @@ public class Version implements Comparable<Version>, ToXContentFragment {
     public static final Version V_1_2_5 = new Version(1020599, org.apache.lucene.util.Version.LUCENE_8_10_1);
     public static final Version V_1_3_0 = new Version(1030099, org.apache.lucene.util.Version.LUCENE_8_10_1);
     public static final Version V_1_3_1 = new Version(1030199, org.apache.lucene.util.Version.LUCENE_8_10_1);
+    public static final Version V_1_3_2 = new Version(1030299, org.apache.lucene.util.Version.LUCENE_8_10_1);
     public static final Version V_1_4_0 = new Version(1040099, org.apache.lucene.util.Version.LUCENE_8_10_1);
     public static final Version V_2_0_0 = new Version(2000099, org.apache.lucene.util.Version.LUCENE_9_1_0);
     public static final Version CURRENT = V_2_0_0;

diff --git a/server/src/main/java/org/opensearch/rest/BaseRestHandler.java b/server/src/main/java/org/opensearch/rest/BaseRestHandler.java
@@ -36,6 +36,7 @@
 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;
@@ -204,26 +205,28 @@ protected Set<String> responseParams() {
 
     /**
      * 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.
+     * 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.
      */
-    @Deprecated
-    protected static void parseDeprecatedMasterTimeoutParameter(
+    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);
-            request.validateParamValuesAreEqual("master_timeout", "cluster_manager_timeout");
+            if (request.hasParam("cluster_manager_timeout")) {
+                throw new OpenSearchParseException(DUPLICATE_PARAMETER_ERROR_MESSAGE);
+            }
             mnr.masterNodeTimeout(request.paramAsTime("master_timeout", mnr.masterNodeTimeout()));
         }
     }

diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java
@@ -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;
@@ -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) {

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,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() {
@@ -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
@@ -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()));
-        }
-    }
 }
diff --git a/server/src/test/java/org/opensearch/action/RenamedTimeoutRequestParameterTests.java b/server/src/test/java/org/opensearch/action/RenamedTimeoutRequestParameterTests.java
@@ -8,10 +8,12 @@
 
 package org.opensearch.action;
 
-import org.junit.AfterClass;
+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.bytes.BytesArray;
+import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.settings.SettingsFilter;
 import org.opensearch.common.xcontent.NamedXContentRegistry;
@@ -22,6 +24,8 @@
 import org.opensearch.rest.action.admin.cluster.RestClusterStateAction;
 import org.opensearch.rest.action.admin.cluster.RestClusterUpdateSettingsAction;
 import org.opensearch.rest.action.admin.cluster.RestPendingClusterTasksAction;
+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;
@@ -38,103 +42,134 @@
  * Remove the test after removing MASTER_ROLE and 'master_timeout'.
  */
 public class RenamedTimeoutRequestParameterTests extends OpenSearchTestCase {
-    private static final TestThreadPool threadPool = new TestThreadPool(RenamedTimeoutRequestParameterTests.class.getName());
+    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 PARAM_VALUE_ERROR_MESSAGE = "[master_timeout, cluster_manager_timeout] are required to be equal";
+    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.";
 
-    @AfterClass
-    public static void terminateThreadPool() {
+    @After
+    public void terminateThreadPool() {
         terminate(threadPool);
     }
 
-    /**
-     * Validate both cluster_manager_timeout and its predecessor can be parsed correctly.
-     */
-    public void testClusterHealth() {
-        RestClusterHealthAction.fromRequest(getRestRequestWithNewParam());
+    public void testNoWarningsForNewParam() {
+        BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
+            getMasterNodeRequest(),
+            getRestRequestWithNewParam(),
+            deprecationLogger,
+            "test"
+        );
+    }
+
+    public void testDeprecationWarningForOldParam() {
+        BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
+            getMasterNodeRequest(),
+            getRestRequestWithDeprecatedParam(),
+            deprecationLogger,
+            "test"
+        );
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
 
-        RestClusterHealthAction.fromRequest(getRestRequestWithDeprecatedParam());
+    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);
+    }
 
+    public void testClusterHealth() {
         Exception e = assertThrows(
             OpenSearchParseException.class,
-            () -> RestClusterHealthAction.fromRequest(getRestRequestWithWrongValues())
+            () -> RestClusterHealthAction.fromRequest(getRestRequestWithBodyWithBothParams())
         );
-        assertThat(e.getMessage(), containsString(PARAM_VALUE_ERROR_MESSAGE));
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
     }
 
     public void testClusterReroute() throws IOException {
         final SettingsFilter filter = new SettingsFilter(Collections.singleton("foo.filtered"));
         RestClusterRerouteAction action = new RestClusterRerouteAction(filter);
-
-        action.prepareRequest(getRestRequestWithNewParam(), client);
-
-        action.prepareRequest(getRestRequestWithDeprecatedParam(), client);
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
         assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
-
-        Exception e = assertThrows(OpenSearchParseException.class, () -> action.prepareRequest(getRestRequestWithWrongValues(), client));
-        assertThat(e.getMessage(), containsString(PARAM_VALUE_ERROR_MESSAGE));
     }
 
     public void testClusterState() throws IOException {
         final SettingsFilter filter = new SettingsFilter(Collections.singleton("foo.filtered"));
         RestClusterStateAction action = new RestClusterStateAction(filter);
-
-        action.prepareRequest(getRestRequestWithNewParam(), client);
-
-        action.prepareRequest(getRestRequestWithDeprecatedParam(), client);
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
         assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
-
-        Exception e = assertThrows(OpenSearchParseException.class, () -> action.prepareRequest(getRestRequestWithWrongValues(), client));
-        assertThat(e.getMessage(), containsString(PARAM_VALUE_ERROR_MESSAGE));
     }
 
     public void testClusterGetSettings() throws IOException {
         final SettingsFilter filter = new SettingsFilter(Collections.singleton("foo.filtered"));
         RestClusterGetSettingsAction action = new RestClusterGetSettingsAction(null, null, filter);
-
-        action.prepareRequest(getRestRequestWithNewParam(), client);
-
-        action.prepareRequest(getRestRequestWithDeprecatedParam(), client);
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
         assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
-
-        Exception e = assertThrows(OpenSearchParseException.class, () -> action.prepareRequest(getRestRequestWithWrongValues(), client));
-        assertThat(e.getMessage(), containsString(PARAM_VALUE_ERROR_MESSAGE));
     }
 
     public void testClusterUpdateSettings() throws IOException {
         RestClusterUpdateSettingsAction action = new RestClusterUpdateSettingsAction();
-
-        action.prepareRequest(getRestRequestWithBodyWithNewParam(), client);
-
-        action.prepareRequest(getRestRequestWithBodyWithDeprecatedParam(), client);
-        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
-
         Exception e = assertThrows(
             OpenSearchParseException.class,
-            () -> action.prepareRequest(getRestRequestWithBodyWithWrongValues(), client)
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
         );
-        assertThat(e.getMessage(), containsString(PARAM_VALUE_ERROR_MESSAGE));
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
     }
 
-    public void testClusterPendingTasks() throws IOException {
+    public void testClusterPendingTasks() {
         RestPendingClusterTasksAction action = new RestPendingClusterTasksAction();
-
-        action.prepareRequest(getRestRequestWithNewParam(), client);
-
-        action.prepareRequest(getRestRequestWithDeprecatedParam(), client);
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
         assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
 
-        Exception e = assertThrows(OpenSearchParseException.class, () -> action.prepareRequest(getRestRequestWithWrongValues(), client));
-        assertThat(e.getMessage(), containsString(PARAM_VALUE_ERROR_MESSAGE));
+    private MasterNodeRequest getMasterNodeRequest() {
+        return new MasterNodeRequest() {
+            @Override
+            public ActionRequestValidationException validate() {
+                return null;
+            }
+        };
     }
 
-    private FakeRestRequest getRestRequestWithWrongValues() {
+    private FakeRestRequest getRestRequestWithBothParams() {
         FakeRestRequest request = new FakeRestRequest();
-        request.params().put("cluster_manager_timeout", randomFrom("1h", "2m"));
+        request.params().put("cluster_manager_timeout", "1h");
         request.params().put("master_timeout", "3s");
         return request;
     }
@@ -151,25 +186,13 @@ private FakeRestRequest getRestRequestWithNewParam() {
         return request;
     }
 
-    private FakeRestRequest getRestRequestWithBodyWithWrongValues() {
+    private FakeRestRequest getRestRequestWithBodyWithBothParams() {
         FakeRestRequest request = getFakeRestRequestWithBody();
         request.params().put("cluster_manager_timeout", randomFrom("1h", "2m"));
         request.params().put("master_timeout", "3s");
         return request;
     }
 
-    private FakeRestRequest getRestRequestWithBodyWithDeprecatedParam() {
-        FakeRestRequest request = getFakeRestRequestWithBody();
-        request.params().put("master_timeout", "3s");
-        return request;
-    }
-
-    private FakeRestRequest getRestRequestWithBodyWithNewParam() {
-        FakeRestRequest request = getFakeRestRequestWithBody();
-        request.params().put("cluster_manager_timeout", "2m");
-        return request;
-    }
-
     private FakeRestRequest getFakeRestRequestWithBody() {
         return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray("{}"), XContentType.JSON).build();
     }
-Original file line number
+Diff line change
@@ Expand Up / @@ -38,4 +38,5 @@ BWC_VERSION: @@
       - "1.2.5"
       - "1.3.0"
       - "1.3.1"
+      - "1.3.2"
       - "1.4.0"