Add request parameter 'cluster_manager_timeout' and deprecate 'master…

…_timeout' - in Cluster APIs (#2658) - Deprecate the request parameter `master_timeout` that used in Cluster APIs which have got the parameter. - Add alternative new request parameter `cluster_manager_timeout`. - Add unit tests. Signed-off-by: Tianli Feng <[email protected]>
opensearch-project · Apr 4, 2022 · 7d23b18 · 7d23b18
1 parent 2d716ad
commit 7d23b18
Show file tree

Hide file tree

Showing 15 changed files with 172 additions and 20 deletions.
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.get_settings.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.get_settings.json
@@ -22,7 +22,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"
       },
       "timeout":{
         "type":"time",

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.health.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.health.json
@@ -56,7 +56,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"
       },
       "timeout":{
         "type":"time",

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.pending_tasks.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.pending_tasks.json
@@ -22,7 +22,15 @@
       },
       "master_timeout":{
         "type":"time",
-        "description":"Specify timeout for connection to master"
+        "description":"Specify timeout for connection to master",
+        "deprecated":{
+          "version":"2.0.0",
+          "description":"To promote inclusive language, use 'cluster_manager_timeout' instead."
+        }
+      },
+      "cluster_manager_timeout":{
+        "type":"time",
+        "description":"Specify timeout for connection to cluster-manager node"
       }
     }
   }

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.put_settings.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.put_settings.json
@@ -22,7 +22,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"
       },
       "timeout":{
         "type":"time",

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.reroute.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.reroute.json
@@ -44,7 +44,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"
       },
       "timeout":{
         "type":"time",

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.state.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.state.json
@@ -71,7 +71,15 @@
       },
       "master_timeout":{
         "type":"time",
-        "description":"Specify timeout for connection to master"
+        "description":"Specify timeout for connection to master",
+        "deprecated":{
+          "version":"2.0.0",
+          "description":"To promote inclusive language, use 'cluster_manager_timeout' instead."
+        }
+      },
+      "cluster_manager_timeout":{
+        "type":"time",
+        "description":"Specify timeout for connection to cluster-manager node"
       },
       "flat_settings":{
         "type":"boolean",

diff --git a/.../src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterGetSettingsAction.java b/.../src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterGetSettingsAction.java
@@ -38,6 +38,7 @@
 import org.opensearch.client.Requests;
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.cluster.ClusterState;
+import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.settings.SettingsFilter;
@@ -59,6 +60,8 @@
 
 public class RestClusterGetSettingsAction extends BaseRestHandler {
 
+    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterGetSettingsAction.class);
+
     private final Settings settings;
     private final ClusterSettings clusterSettings;
     private final SettingsFilter settingsFilter;
@@ -84,7 +87,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
         ClusterStateRequest clusterStateRequest = Requests.clusterStateRequest().routingTable(false).nodes(false);
         final boolean renderDefaults = request.paramAsBoolean("include_defaults", false);
         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, deprecationLogger, getName());
         return channel -> client.admin().cluster().state(clusterStateRequest, new RestBuilderListener<ClusterStateResponse>(channel) {
             @Override
             public RestResponse buildResponse(ClusterStateResponse response, XContentBuilder builder) throws Exception {

diff --git a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterHealthAction.java b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterHealthAction.java
@@ -39,6 +39,7 @@
 import org.opensearch.cluster.health.ClusterHealthStatus;
 import org.opensearch.common.Priority;
 import org.opensearch.common.Strings;
+import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.rest.BaseRestHandler;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.rest.action.RestStatusToXContentListener;
@@ -56,6 +57,8 @@
 
 public class RestClusterHealthAction extends BaseRestHandler {
 
+    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterHealthAction.class);
+
     @Override
     public List<Route> routes() {
         return unmodifiableList(asList(new Route(GET, "/_cluster/health"), new Route(GET, "/_cluster/health/{index}")));
@@ -81,7 +84,8 @@ public static ClusterHealthRequest fromRequest(final RestRequest request) {
         final ClusterHealthRequest clusterHealthRequest = clusterHealthRequest(Strings.splitStringByCommaToArray(request.param("index")));
         clusterHealthRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterHealthRequest.indicesOptions()));
         clusterHealthRequest.local(request.paramAsBoolean("local", clusterHealthRequest.local()));
-        clusterHealthRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterHealthRequest.masterNodeTimeout()));
+        clusterHealthRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterHealthRequest.masterNodeTimeout()));
+        parseDeprecatedMasterTimeoutParameter(clusterHealthRequest, request, deprecationLogger, "cluster_health");
         clusterHealthRequest.timeout(request.paramAsTime("timeout", clusterHealthRequest.timeout()));
         String waitForStatus = request.param("wait_for_status");
         if (waitForStatus != null) {

diff --git a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterRerouteAction.java b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterRerouteAction.java
@@ -80,7 +80,7 @@ public RestClusterRerouteAction(SettingsFilter settingsFilter) {
     }
 
     // TODO: Remove the DeprecationLogger after removing MASTER_ROLE.
-    // It's used to log deprecation when request parameter 'metric' contains 'master_node'.
+    // It's used to log deprecation when request parameter 'metric' contains 'master_node', or request parameter 'master_timeout' is used.
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterRerouteAction.class);
     private static final String DEPRECATED_MESSAGE_MASTER_NODE =
         "Deprecated value [master_node] used for parameter [metric]. To promote inclusive language, please use [cluster_manager_node] instead. It will be unsupported in a future major version.";
@@ -143,7 +143,8 @@ public static ClusterRerouteRequest createRequest(RestRequest request) throws IO
         clusterRerouteRequest.explain(request.paramAsBoolean("explain", clusterRerouteRequest.explain()));
         clusterRerouteRequest.timeout(request.paramAsTime("timeout", clusterRerouteRequest.timeout()));
         clusterRerouteRequest.setRetryFailed(request.paramAsBoolean("retry_failed", clusterRerouteRequest.isRetryFailed()));
-        clusterRerouteRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterRerouteRequest.masterNodeTimeout()));
+        clusterRerouteRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterRerouteRequest.masterNodeTimeout()));
+        parseDeprecatedMasterTimeoutParameter(clusterRerouteRequest, request, deprecationLogger, "cluster_reroute");
         request.applyContentParser(parser -> PARSER.parse(parser, clusterRerouteRequest, null));
         return clusterRerouteRequest;
     }

diff --git a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterStateAction.java b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterStateAction.java
@@ -73,7 +73,7 @@ public RestClusterStateAction(SettingsFilter settingsFilter) {
     }
 
     // TODO: Remove the DeprecationLogger after removing MASTER_ROLE.
-    // It's used to log deprecation when request parameter 'metric' contains 'master_node'.
+    // It's used to log deprecation when request parameter 'metric' contains 'master_node', or request parameter 'master_timeout' is used.
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterStateAction.class);
     private static final String DEPRECATED_MESSAGE_MASTER_NODE =
         "Deprecated value [master_node] used for parameter [metric]. To promote inclusive language, please use [cluster_manager_node] instead. It will be unsupported in a future major version.";
@@ -104,7 +104,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
         final ClusterStateRequest clusterStateRequest = Requests.clusterStateRequest();
         clusterStateRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterStateRequest.indicesOptions()));
         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, deprecationLogger, getName());
         if (request.hasParam("wait_for_metadata_version")) {
             clusterStateRequest.waitForMetadataVersion(request.paramAsLong("wait_for_metadata_version", 0));
         }

diff --git a/...c/main/java/org/opensearch/rest/action/admin/cluster/RestClusterUpdateSettingsAction.java b/...c/main/java/org/opensearch/rest/action/admin/cluster/RestClusterUpdateSettingsAction.java
@@ -35,6 +35,7 @@
 import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
 import org.opensearch.client.Requests;
 import org.opensearch.client.node.NodeClient;
+import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.xcontent.XContentParser;
 import org.opensearch.rest.BaseRestHandler;
@@ -51,6 +52,8 @@
 
 public class RestClusterUpdateSettingsAction extends BaseRestHandler {
 
+    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestClusterUpdateSettingsAction.class);
+
     private static final String PERSISTENT = "persistent";
     private static final String TRANSIENT = "transient";
 
@@ -69,8 +72,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
         final ClusterUpdateSettingsRequest clusterUpdateSettingsRequest = Requests.clusterUpdateSettingsRequest();
         clusterUpdateSettingsRequest.timeout(request.paramAsTime("timeout", clusterUpdateSettingsRequest.timeout()));
         clusterUpdateSettingsRequest.masterNodeTimeout(
-            request.paramAsTime("master_timeout", clusterUpdateSettingsRequest.masterNodeTimeout())
+            request.paramAsTime("cluster_manager_timeout", clusterUpdateSettingsRequest.masterNodeTimeout())
         );
+        parseDeprecatedMasterTimeoutParameter(clusterUpdateSettingsRequest, request, deprecationLogger, getName());
         Map<String, Object> source;
         try (XContentParser parser = request.contentParser()) {
             source = parser.map();

diff --git a/...src/main/java/org/opensearch/rest/action/admin/cluster/RestPendingClusterTasksAction.java b/...src/main/java/org/opensearch/rest/action/admin/cluster/RestPendingClusterTasksAction.java
@@ -34,6 +34,7 @@
 
 import org.opensearch.action.admin.cluster.tasks.PendingClusterTasksRequest;
 import org.opensearch.client.node.NodeClient;
+import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.rest.BaseRestHandler;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.rest.action.RestToXContentListener;
@@ -46,6 +47,8 @@
 
 public class RestPendingClusterTasksAction extends BaseRestHandler {
 
+    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestPendingClusterTasksAction.class);
+
     @Override
     public List<Route> routes() {
         return singletonList(new Route(GET, "/_cluster/pending_tasks"));
@@ -59,7 +62,10 @@ public String getName() {
     @Override
     public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
         PendingClusterTasksRequest pendingClusterTasksRequest = new PendingClusterTasksRequest();
-        pendingClusterTasksRequest.masterNodeTimeout(request.paramAsTime("master_timeout", pendingClusterTasksRequest.masterNodeTimeout()));
+        pendingClusterTasksRequest.masterNodeTimeout(
+            request.paramAsTime("cluster_manager_timeout", pendingClusterTasksRequest.masterNodeTimeout())
+        );
+        parseDeprecatedMasterTimeoutParameter(pendingClusterTasksRequest, request, deprecationLogger, getName());
         pendingClusterTasksRequest.local(request.paramAsBoolean("local", pendingClusterTasksRequest.local()));
         return channel -> client.admin().cluster().pendingClusterTasks(pendingClusterTasksRequest, new RestToXContentListener<>(channel));
     }

diff --git a/server/src/test/java/org/opensearch/action/RenamedTimeoutRequestParameterTests.java b/server/src/test/java/org/opensearch/action/RenamedTimeoutRequestParameterTests.java
@@ -12,8 +12,18 @@
 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;
+import org.opensearch.common.xcontent.XContentType;
+import org.opensearch.rest.BaseRestHandler;
+import org.opensearch.rest.action.admin.cluster.RestClusterGetSettingsAction;
+import org.opensearch.rest.action.admin.cluster.RestClusterHealthAction;
+import org.opensearch.rest.action.admin.cluster.RestClusterRerouteAction;
+import org.opensearch.rest.action.admin.cluster.RestClusterStateAction;
+import org.opensearch.rest.action.admin.cluster.RestClusterUpdateSettingsAction;
 import org.opensearch.rest.action.cat.RestAllocationAction;
 import org.opensearch.rest.action.cat.RestRepositoriesAction;
 import org.opensearch.rest.action.cat.RestThreadPoolAction;
@@ -27,12 +37,13 @@
 import org.opensearch.rest.action.cat.RestPendingClusterTasksAction;
 import org.opensearch.rest.action.cat.RestSegmentsAction;
 import org.opensearch.rest.action.cat.RestSnapshotAction;
-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 java.io.IOException;
+import java.util.Collections;
+
 import static org.hamcrest.Matchers.containsString;
 
 /**
@@ -180,6 +191,68 @@ public void testCatSegments() {
         assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
     }
 
+    public void testClusterHealth() {
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> RestClusterHealthAction.fromRequest(getRestRequestWithBodyWithBothParams())
+        );
+        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);
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
+    public void testClusterState() throws IOException {
+        final SettingsFilter filter = new SettingsFilter(Collections.singleton("foo.filtered"));
+        RestClusterStateAction action = new RestClusterStateAction(filter);
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
+    public void testClusterGetSettings() throws IOException {
+        final SettingsFilter filter = new SettingsFilter(Collections.singleton("foo.filtered"));
+        RestClusterGetSettingsAction action = new RestClusterGetSettingsAction(null, null, filter);
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
+    public void testClusterUpdateSettings() throws IOException {
+        RestClusterUpdateSettingsAction action = new RestClusterUpdateSettingsAction();
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
+    public void testClusterPendingTasks() {
+        RestPendingClusterTasksAction action = new RestPendingClusterTasksAction();
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> action.prepareRequest(getRestRequestWithBodyWithBothParams(), client)
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
     private MasterNodeRequest getMasterNodeRequest() {
         return new MasterNodeRequest() {
             @Override
@@ -207,4 +280,15 @@ private FakeRestRequest getRestRequestWithNewParam() {
         request.params().put("cluster_manager_timeout", "2m");
         return request;
     }
+
+    private FakeRestRequest getRestRequestWithBodyWithBothParams() {
+        FakeRestRequest request = getFakeRestRequestWithBody();
+        request.params().put("cluster_manager_timeout", "2m");
+        request.params().put("master_timeout", "3s");
+        return request;
+    }
+
+    private FakeRestRequest getFakeRestRequestWithBody() {
+        return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray("{}"), XContentType.JSON).build();
+    }
 }
diff --git a/...src/test/java/org/opensearch/action/admin/cluster/reroute/ClusterRerouteRequestTests.java b/...src/test/java/org/opensearch/action/admin/cluster/reroute/ClusterRerouteRequestTests.java
@@ -232,7 +232,7 @@ private RestRequest toRestRequest(ClusterRerouteRequest original) throws IOExcep
             params.put("retry_failed", Boolean.toString(original.isRetryFailed()));
         }
         if (false == original.masterNodeTimeout().equals(MasterNodeRequest.DEFAULT_MASTER_NODE_TIMEOUT) || randomBoolean()) {
-            params.put("master_timeout", original.masterNodeTimeout().toString());
+            params.put("cluster_manager_timeout", original.masterNodeTimeout().toString());
         }
         if (original.getCommands() != null) {
             hasBody = true;