Deprecate setting cluster.service.slow_master_task_logging_threshold

Signed-off-by: Tianli Feng <[email protected]>

server/src/main/java/org/opensearch/cluster/service/MasterService.java

@@ -90,6 +90,15 @@ public class MasterService extends AbstractLifecycleComponent {
         "cluster.service.slow_master_task_logging_threshold",
         TimeValue.timeValueSeconds(10),
         Setting.Property.Dynamic,
+        Setting.Property.NodeScope,
+        Setting.Property.Deprecated
+    );
+    // The setting below is going to replace the above.
+    // To keep backwards compatibility, the old usage is remained, and it's also used as the fallback for the new usage.
+    public static final Setting<TimeValue> CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING = Setting.positiveTimeSetting(
+        "cluster.service.slow_cluster_manager_task_logging_threshold",
+        MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING,
+        Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );
 
@@ -111,8 +120,11 @@ public class MasterService extends AbstractLifecycleComponent {
     public MasterService(Settings settings, ClusterSettings clusterSettings, ThreadPool threadPool) {
         this.nodeName = Objects.requireNonNull(Node.NODE_NAME_SETTING.get(settings));
 
-        this.slowTaskLoggingThreshold = MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(settings);
-        clusterSettings.addSettingsUpdateConsumer(MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING, this::setSlowTaskLoggingThreshold);
+        this.slowTaskLoggingThreshold = CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(settings);
+        clusterSettings.addSettingsUpdateConsumer(
+            CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING,
+            this::setSlowTaskLoggingThreshold
+        );
 
         this.threadPool = threadPool;
     }

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java

@@ -332,7 +332,8 @@ public void apply(Settings value, Settings current, Settings previous) {
                 IndexModule.NODE_STORE_ALLOW_MMAP,
                 ClusterApplierService.CLUSTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING,
                 ClusterService.USER_DEFINED_METADATA,
-                MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING,
+                MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING,  // deprecated
+                MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING,
                 SearchService.DEFAULT_SEARCH_TIMEOUT_SETTING,
                 SearchService.DEFAULT_ALLOW_PARTIAL_SEARCH_RESULTS,
                 TransportSearchAction.SHARD_COUNT_LIMIT_SETTING,

server/src/test/java/org/opensearch/cluster/service/MasterServiceRenamedSettingTests.java

@@ -0,0 +1,96 @@
+/*
+ * 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.cluster.service;
+
+import org.opensearch.common.settings.ClusterSettings;
+import org.opensearch.common.settings.Setting;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.unit.TimeValue;
+import org.opensearch.test.OpenSearchTestCase;
+
+import java.util.Arrays;
+import java.util.Set;
+
+/**
+ * A unit test to validate the former name of the setting 'cluster.service.slow_cluster_manager_task_logging_threshold' still take effect,
+ * after it is deprecated, so that the backwards compatibility is maintained.
+ * The test can be removed along with removing support of the deprecated setting.
+ */
+public class MasterServiceRenamedSettingTests extends OpenSearchTestCase {
+
+    /**
+     * Validate the both settings are known and supported.
+     */
+    public void testClusterManagerServiceSettingsExist() {
+        Set<Setting<?>> settings = ClusterSettings.BUILT_IN_CLUSTER_SETTINGS;
+        assertTrue(
+            "Both 'cluster.service.slow_cluster_manager_task_logging_threshold' and its predecessor should be supported built-in settings",
+            settings.containsAll(
+                Arrays.asList(
+                    MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING,
+                    MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING
+                )
+            )
+        );
+    }
+
+    /**
+     * Validate the default value of the both settings is the same.
+     */
+    public void testSettingFallback() {
+        assertEquals(
+            MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY),
+            MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
+        );
+    }
+
+    /**
+     * Validate the new setting can be configured correctly, and it doesn't impact the old setting.
+     */
+    public void testSettingGetValue() {
+        Settings settings = Settings.builder().put("cluster.service.slow_cluster_manager_task_logging_threshold", "9s").build();
+        assertEquals(
+            MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(settings),
+            TimeValue.timeValueSeconds(9)
+        );
+        assertEquals(
+            MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(settings),
+            MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.getDefault(Settings.EMPTY)
+        );
+    }
+
+    /**
+     * Validate the value of the old setting will be applied to the new setting, if the new setting is not configured.
+     */
+    public void testSettingGetValueWithFallback() {
+        Settings settings = Settings.builder().put("cluster.service.slow_master_task_logging_threshold", "8s").build();
+        assertEquals(
+            MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(settings),
+            TimeValue.timeValueSeconds(8)
+        );
+        assertSettingDeprecationsAndWarnings(new Setting<?>[] { MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING });
+    }
+
+    /**
+     * Validate the value of the old setting will be ignored, if the new setting is configured.
+     */
+    public void testSettingGetValueWhenBothAreConfigured() {
+        Settings settings = Settings.builder()
+            .put("cluster.service.slow_cluster_manager_task_logging_threshold", "9s")
+            .put("cluster.service.slow_master_task_logging_threshold", "8s")
+            .build();
+        assertEquals(
+            MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(settings),
+            TimeValue.timeValueSeconds(9)
+        );
+        assertEquals(MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(settings), TimeValue.timeValueSeconds(8));
+        assertSettingDeprecationsAndWarnings(new Setting<?>[] { MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING });
+    }
+
+}

server/src/test/java/org/opensearch/cluster/service/MasterServiceTests.java

@@ -809,12 +809,14 @@ public void testLongClusterStateUpdateLogging() throws Exception {
                 final AtomicReference<ClusterState> clusterStateRef = new AtomicReference<>(initialClusterState);
                 masterService.setClusterStatePublisher((event, publishListener, ackListener) -> {
                     if (event.source().contains("test5")) {
-                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                            .millis() + randomLongBetween(1, 1000000);
+                        relativeTimeInMillis += MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(
+                            Settings.EMPTY
+                        ).millis() + randomLongBetween(1, 1000000);
                     }
                     if (event.source().contains("test6")) {
-                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                            .millis() + randomLongBetween(1, 1000000);
+                        relativeTimeInMillis += MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(
+                            Settings.EMPTY
+                        ).millis() + randomLongBetween(1, 1000000);
                         throw new OpenSearchException("simulated error during slow publication which should trigger logging");
                     }
                     clusterStateRef.set(event.state());
@@ -830,7 +832,7 @@ public void testLongClusterStateUpdateLogging() throws Exception {
                     public ClusterState execute(ClusterState currentState) {
                         relativeTimeInMillis += randomLongBetween(
                             0L,
-                            MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY).millis()
+                            MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY).millis()
                         );
                         return currentState;
                     }
@@ -851,8 +853,9 @@ public void onFailure(String source, Exception e) {
                 masterService.submitStateUpdateTask("test2", new ClusterStateUpdateTask() {
                     @Override
                     public ClusterState execute(ClusterState currentState) {
-                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                            .millis() + randomLongBetween(1, 1000000);
+                        relativeTimeInMillis += MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(
+                            Settings.EMPTY
+                        ).millis() + randomLongBetween(1, 1000000);
                         throw new IllegalArgumentException("Testing handling of exceptions in the cluster state task");
                     }
 
@@ -869,8 +872,9 @@ public void onFailure(String source, Exception e) {
                 masterService.submitStateUpdateTask("test3", new ClusterStateUpdateTask() {
                     @Override
                     public ClusterState execute(ClusterState currentState) {
-                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                            .millis() + randomLongBetween(1, 1000000);
+                        relativeTimeInMillis += MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(
+                            Settings.EMPTY
+                        ).millis() + randomLongBetween(1, 1000000);
                         return ClusterState.builder(currentState).incrementVersion().build();
                     }
 
@@ -887,8 +891,9 @@ public void onFailure(String source, Exception e) {
                 masterService.submitStateUpdateTask("test4", new ClusterStateUpdateTask() {
                     @Override
                     public ClusterState execute(ClusterState currentState) {
-                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                            .millis() + randomLongBetween(1, 1000000);
+                        relativeTimeInMillis += MasterService.CLUSTER_MANAGER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(
+                            Settings.EMPTY
+                        ).millis() + randomLongBetween(1, 1000000);
                         return currentState;
                     }