Detect and optimize noop of update index settings (#61348)

This optimization is more relevant in the context of CCR. When a node in
the follower cluster leaves, we reallocate the shard-follow tasks on 
that node to other nodes. The new tasks will overwhelm the follower
cluster with many put-mapping, update-settings requests, although most
of them are noop. This change detects and optimizes the noop
update-settings requests.

Author: dnhatn

server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java

@@ -21,10 +21,13 @@
 
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse;
+import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.index.IndexModule;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.VersionType;
@@ -687,4 +690,52 @@ private void runTestDefaultNumberOfReplicasTest(final boolean closeIndex) {
         assertThat(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(response.getIndexToSettings().get("test")), equalTo(1));
     }
 
+    public void testNoopUpdate() {
+        internalCluster().ensureAtLeastNumDataNodes(2);
+        final ClusterService clusterService = internalCluster().getMasterNodeInstance(ClusterService.class);
+        assertAcked(client().admin().indices().prepareCreate("test")
+            .setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)));
+
+        ClusterState currentState = clusterService.state();
+        assertAcked(client().admin().indices().prepareUpdateSettings("test")
+            .setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)));
+        assertNotSame(currentState, clusterService.state());
+        client().admin().cluster().prepareHealth()
+            .setWaitForGreenStatus()
+            .setWaitForNoInitializingShards(true)
+            .setWaitForEvents(Priority.LANGUID)
+            .setTimeout(TimeValue.MAX_VALUE)
+            .get();
+        currentState = clusterService.state();
+
+        assertAcked(client().admin().indices().prepareUpdateSettings("test")
+            .setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)));
+        assertSame(clusterService.state(), currentState);
+
+        assertAcked(client().admin().indices().prepareUpdateSettings("test")
+            .setSettings(Settings.builder().putNull(IndexMetadata.SETTING_NUMBER_OF_REPLICAS)));
+        assertSame(clusterService.state(), currentState);
+
+        assertAcked(client().admin().indices().prepareUpdateSettings("test")
+            .setSettings(Settings.builder()
+                .putNull(SETTING_BLOCKS_READ)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)));
+        assertSame(currentState, clusterService.state());
+
+        assertAcked(client().admin().indices().prepareUpdateSettings("test")
+            .setSettings(Settings.builder()
+                .put(SETTING_BLOCKS_READ, true)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)));
+        assertNotSame(currentState, clusterService.state());
+        currentState = clusterService.state();
+
+        assertAcked(client().admin().indices().prepareUpdateSettings("test")
+            .setSettings(Settings.builder().put(SETTING_BLOCKS_READ, true)));
+        assertSame(currentState, clusterService.state());
+
+        assertAcked(client().admin().indices().prepareUpdateSettings("test")
+            .setSettings(Settings.builder().putNull(SETTING_BLOCKS_READ)));
+        assertNotSame(currentState, clusterService.state());
+    }
+
 }

server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java

@@ -31,6 +31,7 @@
 import org.elasticsearch.rest.RestStatus;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.EnumMap;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -405,6 +406,10 @@ public Builder removeIndexBlocks(String index) {
             return this;
         }
 
+        public boolean hasIndexBlock(String index, ClusterBlock block) {
+            return indices.getOrDefault(index, Collections.emptySet()).contains(block);
+        }
+
         public Builder removeIndexBlock(String index, ClusterBlock block) {
             if (!indices.containsKey(index)) {
                 return this;

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java

@@ -177,11 +177,6 @@ public ClusterState execute(ClusterState currentState) {
                     }
                 }
 
-                ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks());
-                for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) {
-                    maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings);
-                }
-
                 if (!openIndices.isEmpty()) {
                     for (Index index : openIndices) {
                         IndexMetadata indexMetadata = metadataBuilder.getSafe(index);
@@ -246,15 +241,26 @@ public ClusterState execute(ClusterState currentState) {
                         MetadataCreateIndexService.validateTranslogRetentionSettings(metadataBuilder.get(index).getSettings());
                     }
                 }
+                boolean changed = false;
                 // increment settings versions
                 for (final String index : actualIndices) {
                     if (same(currentState.metadata().index(index).getSettings(), metadataBuilder.get(index).getSettings()) == false) {
+                        changed = true;
                         final IndexMetadata.Builder builder = IndexMetadata.builder(metadataBuilder.get(index));
                         builder.settingsVersion(1 + builder.settingsVersion());
                         metadataBuilder.put(builder);
                     }
                 }
 
+                final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks());
+                for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) {
+                    changed |= maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings);
+                }
+
+                if (changed == false) {
+                    return currentState;
+                }
+
                 ClusterState updatedState = ClusterState.builder(currentState).metadata(metadataBuilder)
                     .routingTable(routingTableBuilder.build()).blocks(blocks).build();
 
@@ -294,18 +300,26 @@ private int getTotalNewShards(Index index, ClusterState currentState, int update
     /**
      * Updates the cluster block only iff the setting exists in the given settings
      */
-    private static void maybeUpdateClusterBlock(String[] actualIndices, ClusterBlocks.Builder blocks, ClusterBlock block,
+    private static boolean maybeUpdateClusterBlock(String[] actualIndices, ClusterBlocks.Builder blocks, ClusterBlock block,
                                                 Setting<Boolean> setting, Settings openSettings) {
+        boolean changed = false;
         if (setting.exists(openSettings)) {
             final boolean updateBlock = setting.get(openSettings);
             for (String index : actualIndices) {
                 if (updateBlock) {
-                    blocks.addIndexBlock(index, block);
+                    if (blocks.hasIndexBlock(index, block) == false) {
+                        blocks.addIndexBlock(index, block);
+                        changed = true;
+                    }
                 } else {
-                    blocks.removeIndexBlock(index, block);
+                    if (blocks.hasIndexBlock(index, block)) {
+                        blocks.removeIndexBlock(index, block);
+                        changed = true;
+                    }
                 }
             }
         }
+        return changed;
     }