From 2edfaa733ffe9cb7372885b473fff2b24e846aab Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 20 Jan 2022 14:59:38 -0500 Subject: [PATCH 01/11] Whitespace --- .../cluster/metadata/MetadataUpdateSettingsService.java | 3 --- 1 file changed, 3 deletions(-) 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 ce6843c71bc89..6cf0a8e65f238 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -50,9 +50,7 @@ public class MetadataUpdateSettingsService { private static final Logger logger = LogManager.getLogger(MetadataUpdateSettingsService.class); private final ClusterService clusterService; - private final AllocationService allocationService; - private final IndexScopedSettings indexScopedSettings; private final IndicesService indicesService; private final ShardLimitValidator shardLimitValidator; @@ -256,7 +254,6 @@ public static void updateIndexSettings( BiFunction settingUpdater, Boolean preserveExisting, IndexScopedSettings indexScopedSettings - ) { for (Index index : indices) { IndexMetadata indexMetadata = metadataBuilder.getSafe(index); From b93782fac2bb9b70d7de730e7d60ea3e8b1dde76 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 20 Jan 2022 15:00:08 -0500 Subject: [PATCH 02/11] Assign these in constructor argument order --- .../cluster/metadata/MetadataUpdateSettingsService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6cf0a8e65f238..58653ca05dede 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -65,11 +65,11 @@ public MetadataUpdateSettingsService( ThreadPool threadPool ) { this.clusterService = clusterService; - this.threadPool = threadPool; this.allocationService = allocationService; this.indexScopedSettings = indexScopedSettings; this.indicesService = indicesService; this.shardLimitValidator = shardLimitValidator; + this.threadPool = threadPool; } public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request, final ActionListener listener) { From 11ca16405b22a679cabdbcc3ba6655abd32714f6 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 12 Jan 2022 16:50:01 -0500 Subject: [PATCH 03/11] Just use metadata() here --- .../cluster/metadata/MetadataUpdateSettingsService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 58653ca05dede..bbb0f3a7850b8 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -225,12 +225,12 @@ public ClusterState execute(ClusterState currentState) { updatedState = allocationService.reroute(updatedState, "settings update"); try { for (Index index : openIndices) { - final IndexMetadata currentMetadata = currentState.getMetadata().getIndexSafe(index); + final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); indicesService.verifyIndexMetadata(currentMetadata, updatedMetadata); } for (Index index : closeIndices) { - final IndexMetadata currentMetadata = currentState.getMetadata().getIndexSafe(index); + final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); // Verifies that the current index settings can be updated with the updated dynamic settings. indicesService.verifyIndexMetadata(currentMetadata, updatedMetadata); From 82fab8cc862085c887ffbfeeac92907ea85c36f3 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 11 Jan 2022 17:06:58 -0500 Subject: [PATCH 04/11] Extract a clusterTask to pass, rather than on-the-fly --- .../MetadataUpdateSettingsService.java | 253 +++++++++--------- 1 file changed, 129 insertions(+), 124 deletions(-) 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 bbb0f3a7850b8..1b2c455ff6196 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -103,147 +103,152 @@ public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request final Settings openSettings = settingsForOpenIndices.build(); final boolean preserveExisting = request.isPreserveExisting(); - clusterService.submitStateUpdateTask( - "update-settings " + Arrays.toString(request.indices()), - new AckedClusterStateUpdateTask(Priority.URGENT, request, wrapPreservingContext(listener, threadPool.getThreadContext())) { - - @Override - public ClusterState execute(ClusterState currentState) { + // TODO: move this to custom class instead of AckedClusterStateUpdateTask + AckedClusterStateUpdateTask clusterTask = new AckedClusterStateUpdateTask( + Priority.URGENT, + request, + wrapPreservingContext(listener, threadPool.getThreadContext()) + ) { + @Override + public ClusterState execute(ClusterState currentState) { + RoutingTable.Builder routingTableBuilder = null; + Metadata.Builder metadataBuilder = Metadata.builder(currentState.metadata()); - RoutingTable.Builder routingTableBuilder = null; - Metadata.Builder metadataBuilder = Metadata.builder(currentState.metadata()); - - // allow to change any settings to a close index, and only allow dynamic settings to be changed - // on an open index - Set openIndices = new HashSet<>(); - Set closeIndices = new HashSet<>(); - final String[] actualIndices = new String[request.indices().length]; - for (int i = 0; i < request.indices().length; i++) { - Index index = request.indices()[i]; - actualIndices[i] = index.getName(); - final IndexMetadata metadata = currentState.metadata().getIndexSafe(index); - if (metadata.getState() == IndexMetadata.State.OPEN) { - openIndices.add(index); - } else { - closeIndices.add(index); - } + // allow to change any settings to a close index, and only allow dynamic settings to be changed + // on an open index + Set openIndices = new HashSet<>(); + Set closeIndices = new HashSet<>(); + final String[] actualIndices = new String[request.indices().length]; + for (int i = 0; i < request.indices().length; i++) { + Index index = request.indices()[i]; + actualIndices[i] = index.getName(); + final IndexMetadata metadata = currentState.metadata().getIndexSafe(index); + if (metadata.getState() == IndexMetadata.State.OPEN) { + openIndices.add(index); + } else { + closeIndices.add(index); } + } - if (skippedSettings.isEmpty() == false && openIndices.isEmpty() == false) { - throw new IllegalArgumentException( - String.format( - Locale.ROOT, - "Can't update non dynamic settings [%s] for open indices %s", - skippedSettings, - openIndices - ) - ); - } + if (skippedSettings.isEmpty() == false && openIndices.isEmpty() == false) { + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "Can't update non dynamic settings [%s] for open indices %s", + skippedSettings, + openIndices + ) + ); + } - if (IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.exists(openSettings)) { - final int updatedNumberOfReplicas = IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(openSettings); - if (preserveExisting == false) { - // Verify that this won't take us over the cluster shard limit. - shardLimitValidator.validateShardLimitOnReplicaUpdate(currentState, request.indices(), updatedNumberOfReplicas); + if (IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.exists(openSettings)) { + final int updatedNumberOfReplicas = IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(openSettings); + if (preserveExisting == false) { + // Verify that this won't take us over the cluster shard limit. + shardLimitValidator.validateShardLimitOnReplicaUpdate(currentState, request.indices(), updatedNumberOfReplicas); - /* - * We do not update the in-sync allocation IDs as they will be removed upon the first index operation - * which makes these copies stale. - * - * TODO: should we update the in-sync allocation IDs once the data is deleted by the node? - */ - routingTableBuilder = RoutingTable.builder(currentState.routingTable()); - routingTableBuilder.updateNumberOfReplicas(updatedNumberOfReplicas, actualIndices); - metadataBuilder.updateNumberOfReplicas(updatedNumberOfReplicas, actualIndices); - logger.info("updating number_of_replicas to [{}] for indices {}", updatedNumberOfReplicas, actualIndices); - } + /* + * We do not update the in-sync allocation IDs as they will be removed upon the first index operation + * which makes these copies stale. + * + * TODO: should we update the in-sync allocation IDs once the data is deleted by the node? + */ + routingTableBuilder = RoutingTable.builder(currentState.routingTable()); + routingTableBuilder.updateNumberOfReplicas(updatedNumberOfReplicas, actualIndices); + metadataBuilder.updateNumberOfReplicas(updatedNumberOfReplicas, actualIndices); + logger.info("updating number_of_replicas to [{}] for indices {}", updatedNumberOfReplicas, actualIndices); } + } - updateIndexSettings( - openIndices, - metadataBuilder, - (index, indexSettings) -> indexScopedSettings.updateDynamicSettings( - openSettings, - indexSettings, - Settings.builder(), - index.getName() - ), - preserveExisting, - indexScopedSettings - ); + updateIndexSettings( + openIndices, + metadataBuilder, + (index, indexSettings) -> indexScopedSettings.updateDynamicSettings( + openSettings, + indexSettings, + Settings.builder(), + index.getName() + ), + preserveExisting, + indexScopedSettings + ); - updateIndexSettings( - closeIndices, - metadataBuilder, - (index, indexSettings) -> indexScopedSettings.updateSettings( - closedSettings, - indexSettings, - Settings.builder(), - index.getName() - ), - preserveExisting, - indexScopedSettings - ); + updateIndexSettings( + closeIndices, + metadataBuilder, + (index, indexSettings) -> indexScopedSettings.updateSettings( + closedSettings, + indexSettings, + Settings.builder(), + index.getName() + ), + preserveExisting, + indexScopedSettings + ); - if (IndexSettings.INDEX_TRANSLOG_RETENTION_AGE_SETTING.exists(normalizedSettings) - || IndexSettings.INDEX_TRANSLOG_RETENTION_SIZE_SETTING.exists(normalizedSettings)) { - for (String index : actualIndices) { - final Settings settings = metadataBuilder.get(index).getSettings(); - MetadataCreateIndexService.validateTranslogRetentionSettings(settings); - MetadataCreateIndexService.validateStoreTypeSetting(settings); - } + if (IndexSettings.INDEX_TRANSLOG_RETENTION_AGE_SETTING.exists(normalizedSettings) + || IndexSettings.INDEX_TRANSLOG_RETENTION_SIZE_SETTING.exists(normalizedSettings)) { + for (String index : actualIndices) { + final Settings settings = metadataBuilder.get(index).getSettings(); + MetadataCreateIndexService.validateTranslogRetentionSettings(settings); + MetadataCreateIndexService.validateStoreTypeSetting(settings); } - 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); - } + } + 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()); - boolean changedBlocks = false; - for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) { - changedBlocks |= maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings); - } - changed |= changedBlocks; + final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); + boolean changedBlocks = false; + for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) { + changedBlocks |= maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings); + } + changed |= changedBlocks; - if (changed == false) { - return currentState; - } + if (changed == false) { + return currentState; + } - ClusterState updatedState = ClusterState.builder(currentState) - .metadata(metadataBuilder) - .routingTable(routingTableBuilder == null ? currentState.routingTable() : routingTableBuilder.build()) - .blocks(changedBlocks ? blocks.build() : currentState.blocks()) - .build(); + ClusterState updatedState = ClusterState.builder(currentState) + .metadata(metadataBuilder) + .routingTable(routingTableBuilder == null ? currentState.routingTable() : routingTableBuilder.build()) + .blocks(changedBlocks ? blocks.build() : currentState.blocks()) + .build(); - // now, reroute in case things change that require it (like number of replicas) - updatedState = allocationService.reroute(updatedState, "settings update"); - try { - for (Index index : openIndices) { - final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); - final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); - indicesService.verifyIndexMetadata(currentMetadata, updatedMetadata); - } - for (Index index : closeIndices) { - final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); - final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); - // Verifies that the current index settings can be updated with the updated dynamic settings. - indicesService.verifyIndexMetadata(currentMetadata, updatedMetadata); - // Now check that we can create the index with the updated settings (dynamic and non-dynamic). - // This step is mandatory since we allow to update non-dynamic settings on closed indices. - indicesService.verifyIndexMetadata(updatedMetadata, updatedMetadata); - } - } catch (IOException ex) { - throw ExceptionsHelper.convertToElastic(ex); + // now, reroute in case things change that require it (like number of replicas) + updatedState = allocationService.reroute(updatedState, "settings update"); + try { + for (Index index : openIndices) { + final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); + final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); + indicesService.verifyIndexMetadata(currentMetadata, updatedMetadata); } - return updatedState; + for (Index index : closeIndices) { + final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); + final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); + // Verifies that the current index settings can be updated with the updated dynamic settings. + indicesService.verifyIndexMetadata(currentMetadata, updatedMetadata); + // Now check that we can create the index with the updated settings (dynamic and non-dynamic). + // This step is mandatory since we allow to update non-dynamic settings on closed indices. + indicesService.verifyIndexMetadata(updatedMetadata, updatedMetadata); + } + } catch (IOException ex) { + throw ExceptionsHelper.convertToElastic(ex); } - }, + return updatedState; + } + }; + + clusterService.submitStateUpdateTask( + "update-settings " + Arrays.toString(request.indices()), + clusterTask, ClusterStateTaskExecutor.unbatched() ); } From 2c0c648b2cd2eea7950cfeac38eaa6cf85965772 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 20 Jan 2022 09:46:18 -0500 Subject: [PATCH 05/11] Move the reroute *slightly* later --- .../cluster/metadata/MetadataUpdateSettingsService.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 1b2c455ff6196..792b16be3458e 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -222,8 +222,6 @@ public ClusterState execute(ClusterState currentState) { .blocks(changedBlocks ? blocks.build() : currentState.blocks()) .build(); - // now, reroute in case things change that require it (like number of replicas) - updatedState = allocationService.reroute(updatedState, "settings update"); try { for (Index index : openIndices) { final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); @@ -242,6 +240,10 @@ public ClusterState execute(ClusterState currentState) { } catch (IOException ex) { throw ExceptionsHelper.convertToElastic(ex); } + + // now, reroute in case things change that require it (like number of replicas) + updatedState = allocationService.reroute(updatedState, "settings update"); + return updatedState; } }; From 86b0913ecc79e2e369aff64493b2527673f4e977 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 20 Jan 2022 15:00:42 -0500 Subject: [PATCH 06/11] Use a reroute-after-tasks-finished executor --- .../MetadataUpdateSettingsService.java | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) 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 792b16be3458e..44125d57025c7 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -17,6 +17,7 @@ import org.elasticsearch.cluster.AckedClusterStateUpdateTask; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateTaskExecutor; +import org.elasticsearch.cluster.ClusterStateTaskExecutor.ClusterTasksResult; import org.elasticsearch.cluster.block.ClusterBlock; import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.routing.RoutingTable; @@ -55,6 +56,7 @@ public class MetadataUpdateSettingsService { private final IndicesService indicesService; private final ShardLimitValidator shardLimitValidator; private final ThreadPool threadPool; + private final ClusterStateTaskExecutor executor; public MetadataUpdateSettingsService( ClusterService clusterService, @@ -70,6 +72,21 @@ public MetadataUpdateSettingsService( this.indicesService = indicesService; this.shardLimitValidator = shardLimitValidator; this.threadPool = threadPool; + this.executor = (currentState, tasks) -> { + ClusterTasksResult.Builder builder = ClusterTasksResult.builder(); + ClusterState state = currentState; + for (AckedClusterStateUpdateTask task : tasks) { + try { + state = task.execute(state); + builder.success(task); + } catch (Exception e) { + builder.failure(task, e); + } + } + // reroute in case things change that require it (like number of replicas) + state = allocationService.reroute(state, "settings update"); + return builder.build(state); + }; } public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request, final ActionListener listener) { @@ -241,18 +258,11 @@ public ClusterState execute(ClusterState currentState) { throw ExceptionsHelper.convertToElastic(ex); } - // now, reroute in case things change that require it (like number of replicas) - updatedState = allocationService.reroute(updatedState, "settings update"); - return updatedState; } }; - clusterService.submitStateUpdateTask( - "update-settings " + Arrays.toString(request.indices()), - clusterTask, - ClusterStateTaskExecutor.unbatched() - ); + clusterService.submitStateUpdateTask("update-settings " + Arrays.toString(request.indices()), clusterTask, this.executor); } public static void updateIndexSettings( From 56f4b9f59442006a45b9f2c381beb7409203b628 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 24 Jan 2022 14:08:28 -0500 Subject: [PATCH 07/11] This gets the tests passing again It's the first thing the reroute call itself actually called, so we're keeping a portion of a full reroute call here, but deferring the rest of it until the end of the executor loop. --- .../cluster/metadata/MetadataUpdateSettingsService.java | 4 ++++ 1 file changed, 4 insertions(+) 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 44125d57025c7..b27b7fd7a5639 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -239,6 +239,10 @@ public ClusterState execute(ClusterState currentState) { .blocks(changedBlocks ? blocks.build() : currentState.blocks()) .build(); + // we need to tweak auto expand replicas in order to avoid tripping assertions in + // AllocationService.reroute(RoutingAllocation allocation) -- this is far from ideal + updatedState = allocationService.adaptAutoExpandReplicas(updatedState); + try { for (Index index : openIndices) { final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); From dc452c7b68edb891d19e09ee626dbd3e5c14a4ed Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 24 Jan 2022 14:09:17 -0500 Subject: [PATCH 08/11] Rename this variable, s/closeIndices/closedIndices/g --- .../metadata/MetadataUpdateSettingsService.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 b27b7fd7a5639..ec999b73e5334 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -131,10 +131,10 @@ public ClusterState execute(ClusterState currentState) { RoutingTable.Builder routingTableBuilder = null; Metadata.Builder metadataBuilder = Metadata.builder(currentState.metadata()); - // allow to change any settings to a close index, and only allow dynamic settings to be changed + // allow to change any settings to a closed index, and only allow dynamic settings to be changed // on an open index Set openIndices = new HashSet<>(); - Set closeIndices = new HashSet<>(); + Set closedIndices = new HashSet<>(); final String[] actualIndices = new String[request.indices().length]; for (int i = 0; i < request.indices().length; i++) { Index index = request.indices()[i]; @@ -143,7 +143,7 @@ public ClusterState execute(ClusterState currentState) { if (metadata.getState() == IndexMetadata.State.OPEN) { openIndices.add(index); } else { - closeIndices.add(index); + closedIndices.add(index); } } @@ -191,7 +191,7 @@ public ClusterState execute(ClusterState currentState) { ); updateIndexSettings( - closeIndices, + closedIndices, metadataBuilder, (index, indexSettings) -> indexScopedSettings.updateSettings( closedSettings, @@ -249,7 +249,7 @@ public ClusterState execute(ClusterState currentState) { final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); indicesService.verifyIndexMetadata(currentMetadata, updatedMetadata); } - for (Index index : closeIndices) { + for (Index index : closedIndices) { final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); final IndexMetadata updatedMetadata = updatedState.metadata().getIndexSafe(index); // Verifies that the current index settings can be updated with the updated dynamic settings. From be4de6ca529d1312e369c1ca24656deb75931e99 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 24 Jan 2022 14:16:26 -0500 Subject: [PATCH 09/11] Skip the reroute if the whole batch was a noop --- .../cluster/metadata/MetadataUpdateSettingsService.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 ec999b73e5334..69f1b392cfa50 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -83,8 +83,10 @@ public MetadataUpdateSettingsService( builder.failure(task, e); } } - // reroute in case things change that require it (like number of replicas) - state = allocationService.reroute(state, "settings update"); + if (state != currentState) { + // reroute in case things change that require it (like number of replicas) + state = allocationService.reroute(state, "settings update"); + } return builder.build(state); }; } From 92bc39df8a4e884cd649c5e43f50f5359cc324e9 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 25 Jan 2022 11:32:58 -0500 Subject: [PATCH 10/11] Rework test code, remove adaptAutoExpandReplicas hack --- .../MetadataUpdateSettingsService.java | 4 ---- .../indices/cluster/ClusterStateChanges.java | 20 ++++++++++++++----- 2 files changed, 15 insertions(+), 9 deletions(-) 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 69f1b392cfa50..ae06e6f6f9636 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -241,10 +241,6 @@ public ClusterState execute(ClusterState currentState) { .blocks(changedBlocks ? blocks.build() : currentState.blocks()) .build(); - // we need to tweak auto expand replicas in order to avoid tripping assertions in - // AllocationService.reroute(RoutingAllocation allocation) -- this is far from ideal - updatedState = allocationService.adaptAutoExpandReplicas(updatedState); - try { for (Index index : openIndices) { final IndexMetadata currentMetadata = currentState.metadata().getIndexSafe(index); diff --git a/server/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java b/server/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java index a501460841c7d..121c4a6f71501 100644 --- a/server/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java +++ b/server/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java @@ -38,6 +38,7 @@ import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateTaskExecutor; import org.elasticsearch.cluster.ClusterStateTaskExecutor.ClusterTasksResult; +import org.elasticsearch.cluster.ClusterStateTaskExecutor.TaskResult; import org.elasticsearch.cluster.ClusterStateUpdateTask; import org.elasticsearch.cluster.EmptyClusterInfoService; import org.elasticsearch.cluster.action.shard.ShardStateAction; @@ -440,7 +441,7 @@ public ClusterState applyStartedShards(ClusterState clusterState, Map ClusterState runTasks(ClusterStateTaskExecutor executor, ClusterState clusterState, List entries) { try { ClusterTasksResult result = executor.execute(clusterState, entries); - for (ClusterStateTaskExecutor.TaskResult taskResult : result.executionResults.values()) { + for (TaskResult taskResult : result.executionResults.values()) { if (taskResult.isSuccess() == false) { throw taskResult.getFailure(); } @@ -465,16 +466,25 @@ private , Response extends ActionResp }); } + @SuppressWarnings("unchecked") private ClusterState executeClusterStateUpdateTask(ClusterState state, Runnable runnable) { - ClusterState[] result = new ClusterState[1]; + ClusterState[] resultingState = new ClusterState[1]; doAnswer(invocationOnMock -> { ClusterStateUpdateTask task = (ClusterStateUpdateTask) invocationOnMock.getArguments()[1]; - result[0] = task.execute(state); + ClusterStateTaskExecutor executor = (ClusterStateTaskExecutor) invocationOnMock + .getArguments()[2]; + ClusterTasksResult result = executor.execute(state, List.of(task)); + for (TaskResult taskResult : result.executionResults.values()) { + if (taskResult.isSuccess() == false) { + throw taskResult.getFailure(); + } + } + resultingState[0] = result.resultingState; return null; }).when(clusterService).submitStateUpdateTask(anyString(), any(ClusterStateUpdateTask.class), any()); runnable.run(); - assertThat(result[0], notNullValue()); - return result[0]; + assertThat(resultingState[0], notNullValue()); + return resultingState[0]; } private ActionListener createTestListener() { From fd89e179b1891d8c42b68b6439f0029d70be8698 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 25 Jan 2022 11:45:31 -0500 Subject: [PATCH 11/11] Update docs/changelog/82896.yaml --- docs/changelog/82896.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/82896.yaml diff --git a/docs/changelog/82896.yaml b/docs/changelog/82896.yaml new file mode 100644 index 0000000000000..8267ce5a55594 --- /dev/null +++ b/docs/changelog/82896.yaml @@ -0,0 +1,6 @@ +pr: 82896 +summary: Batch Index Settings Update Requests +area: Cluster Coordination +type: enhancement +issues: + - 79866