From 0729616f6112b7b3e700fadf22771ee12bd7f0b2 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Wed, 19 Aug 2020 15:07:23 -0400 Subject: [PATCH 1/2] Detect noop of index setting update --- .../indices/settings/UpdateSettingsIT.java | 39 +++++++++++++++++++ .../MetadataUpdateSettingsService.java | 21 +++++++--- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 8e1005a0cb983..6012f587894c6 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/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,40 @@ 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, 2))); + client().admin().cluster().prepareHealth() + .setWaitForNoInitializingShards(true) + .setWaitForEvents(Priority.LANGUID) + .setTimeout(TimeValue.MAX_VALUE) + .get(); + + ClusterState currentState = clusterService.state(); + assertAcked(client().admin().indices().prepareUpdateSettings("test") + .setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1))); + assertNotSame(currentState, clusterService.state()); + 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() + .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))); + assertNotSame(currentState, clusterService.state()); + } } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index a308af9222331..cc1d635af663c 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/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,8 +300,9 @@ 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 setting, Settings openSettings) { + boolean changed = false; if (setting.exists(openSettings)) { final boolean updateBlock = setting.get(openSettings); for (String index : actualIndices) { @@ -304,8 +311,10 @@ private static void maybeUpdateClusterBlock(String[] actualIndices, ClusterBlock } else { blocks.removeIndexBlock(index, block); } + changed = true; } } + return changed; } From f43c2ead444482eebbd402773e847eb1a9e086b6 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Thu, 20 Aug 2020 12:50:26 -0400 Subject: [PATCH 2/2] detect noop in blocks --- .../indices/settings/UpdateSettingsIT.java | 24 ++++++++++++++----- .../cluster/block/ClusterBlocks.java | 5 ++++ .../MetadataUpdateSettingsService.java | 11 ++++++--- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 6012f587894c6..1a2d9aef29096 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -694,17 +694,18 @@ 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, 2))); - client().admin().cluster().prepareHealth() - .setWaitForNoInitializingShards(true) - .setWaitForEvents(Priority.LANGUID) - .setTimeout(TimeValue.MAX_VALUE) - .get(); + .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") @@ -715,6 +716,12 @@ public void testNoopUpdate() { .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) @@ -724,6 +731,11 @@ public void testNoopUpdate() { 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()); } + } diff --git a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java index f10269dc99d2c..b7b4b263b8a41 100644 --- a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java +++ b/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; diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index cc1d635af663c..32bb85f3f67cf 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -307,11 +307,16 @@ private static boolean maybeUpdateClusterBlock(String[] actualIndices, ClusterBl 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; + } } - changed = true; } } return changed;