From da833d6cd3c6a634ac1b897bd95cb0aabfd45191 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 15 May 2020 15:44:03 -0400 Subject: [PATCH] Use settings infrastructure for shards and replicas (#56801) We get the number of shards and replicas with our bare hands in index metadata, rather than letting the settings infrastructure do the work for us. This commit switches to using the settings infrastructure. --- .../cluster/metadata/IndexMetadata.java | 25 +++++----- .../metadata/MetadataCreateIndexService.java | 32 ++++++++----- .../MetadataUpdateSettingsService.java | 12 +++-- .../common/settings/Setting.java | 10 +++- .../cluster/metadata/IndexMetadataTests.java | 48 +++++++++++++++++++ 5 files changed, 95 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java index 1e9510c031c..4049a2f0034 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java @@ -1173,29 +1173,26 @@ public class IndexMetadata implements Diffable, ToXContentFragmen } } - Integer maybeNumberOfShards = settings.getAsInt(SETTING_NUMBER_OF_SHARDS, null); - if (maybeNumberOfShards == null) { - throw new IllegalArgumentException("must specify numberOfShards for index [" + index + "]"); - } - int numberOfShards = maybeNumberOfShards; - if (numberOfShards <= 0) { - throw new IllegalArgumentException("must specify positive number of shards for index [" + index + "]"); + /* + * We expect that the metadata has been properly built to set the number of shards and the number of replicas, and do not rely + * on the default values here. Those must have been set upstream. + */ + if (INDEX_NUMBER_OF_SHARDS_SETTING.exists(settings) == false) { + throw new IllegalArgumentException("must specify number of shards for index [" + index + "]"); } + final int numberOfShards = INDEX_NUMBER_OF_SHARDS_SETTING.get(settings); - Integer maybeNumberOfReplicas = settings.getAsInt(SETTING_NUMBER_OF_REPLICAS, null); - if (maybeNumberOfReplicas == null) { - throw new IllegalArgumentException("must specify numberOfReplicas for index [" + index + "]"); - } - int numberOfReplicas = maybeNumberOfReplicas; - if (numberOfReplicas < 0) { - throw new IllegalArgumentException("must specify non-negative number of replicas for index [" + index + "]"); + if (INDEX_NUMBER_OF_REPLICAS_SETTING.exists(settings) == false) { + throw new IllegalArgumentException("must specify number of replicas for index [" + index + "]"); } + final int numberOfReplicas = INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings); int routingPartitionSize = INDEX_ROUTING_PARTITION_SIZE_SETTING.get(settings); if (routingPartitionSize != 1 && routingPartitionSize >= getRoutingNumShards()) { throw new IllegalArgumentException("routing partition size [" + routingPartitionSize + "] should be a positive number" + " less than the number of shards [" + getRoutingNumShards() + "] for [" + index + "]"); } + // fill missing slots in inSyncAllocationIds with empty set if needed and make all entries immutable ImmutableOpenIntMap.Builder> filledInSyncAllocationIds = ImmutableOpenIntMap.builder(); for (int i = 0; i < numberOfShards; i++) { diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java index 5694fae6971..fb025c3e55f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -101,6 +101,8 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import static java.util.stream.Collectors.toList; +import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING; +import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING; import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS; import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_CREATION_DATE; import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_INDEX_UUID; @@ -720,12 +722,17 @@ public class MetadataCreateIndexService { final Version createdVersion = Version.min(Version.CURRENT, nodes.getSmallestNonClientNodeVersion()); indexSettingsBuilder.put(IndexMetadata.SETTING_INDEX_VERSION_CREATED.getKey(), createdVersion); } - if (indexSettingsBuilder.get(SETTING_NUMBER_OF_SHARDS) == null) { - final int numberOfShards = getNumberOfShards(indexSettingsBuilder); - indexSettingsBuilder.put(SETTING_NUMBER_OF_SHARDS, settings.getAsInt(SETTING_NUMBER_OF_SHARDS, numberOfShards)); + if (INDEX_NUMBER_OF_SHARDS_SETTING.exists(indexSettingsBuilder) == false) { + final int numberOfShards; + if (INDEX_NUMBER_OF_SHARDS_SETTING.exists(settings)) { + numberOfShards = INDEX_NUMBER_OF_SHARDS_SETTING.get(settings); + } else { + numberOfShards = getNumberOfShards(indexSettingsBuilder); + } + indexSettingsBuilder.put(SETTING_NUMBER_OF_SHARDS, numberOfShards); } - if (indexSettingsBuilder.get(SETTING_NUMBER_OF_REPLICAS) == null) { - indexSettingsBuilder.put(SETTING_NUMBER_OF_REPLICAS, settings.getAsInt(SETTING_NUMBER_OF_REPLICAS, 1)); + if (INDEX_NUMBER_OF_REPLICAS_SETTING.exists(indexSettingsBuilder) == false) { + indexSettingsBuilder.put(SETTING_NUMBER_OF_REPLICAS, INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings)); } if (settings.get(SETTING_AUTO_EXPAND_REPLICAS) != null && indexSettingsBuilder.get(SETTING_AUTO_EXPAND_REPLICAS) == null) { indexSettingsBuilder.put(SETTING_AUTO_EXPAND_REPLICAS, settings.get(SETTING_AUTO_EXPAND_REPLICAS)); @@ -784,7 +791,7 @@ public class MetadataCreateIndexService { * it will return the value configured for that index. */ static int getIndexNumberOfRoutingShards(Settings indexSettings, @Nullable IndexMetadata sourceMetadata) { - final int numTargetShards = IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(indexSettings); + final int numTargetShards = INDEX_NUMBER_OF_SHARDS_SETTING.get(indexSettings); final Version indexVersionCreated = IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(indexSettings); final int routingNumShards; if (sourceMetadata == null || sourceMetadata.getNumberOfShards() == 1) { @@ -1010,7 +1017,7 @@ public class MetadataCreateIndexService { * @throws ValidationException if creating this index would put the cluster over the cluster shard limit */ public static void checkShardLimit(final Settings settings, final ClusterState clusterState) { - final int numberOfShards = IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(settings); + final int numberOfShards = INDEX_NUMBER_OF_SHARDS_SETTING.get(settings); final int numberOfReplicas = IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(settings); final int shardsToCreate = numberOfShards * (1 + numberOfReplicas); @@ -1076,8 +1083,8 @@ public class MetadataCreateIndexService { Set targetIndexMappingsTypes, String targetIndexName, Settings targetIndexSettings) { IndexMetadata sourceMetadata = validateResize(state, sourceIndex, targetIndexMappingsTypes, targetIndexName, targetIndexSettings); - assert IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.exists(targetIndexSettings); - IndexMetadata.selectShrinkShards(0, sourceMetadata, IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); + assert INDEX_NUMBER_OF_SHARDS_SETTING.exists(targetIndexSettings); + IndexMetadata.selectShrinkShards(0, sourceMetadata, INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); if (sourceMetadata.getNumberOfShards() == 1) { throw new IllegalArgumentException("can't shrink an index with only one shard"); @@ -1116,14 +1123,13 @@ public class MetadataCreateIndexService { // since in 5.x we don't have a setting to artificially set the number of routing shards throw new IllegalStateException("source index created version is too old to apply a split operation"); } - } static void validateCloneIndex(ClusterState state, String sourceIndex, Set targetIndexMappingsTypes, String targetIndexName, Settings targetIndexSettings) { IndexMetadata sourceMetadata = validateResize(state, sourceIndex, targetIndexMappingsTypes, targetIndexName, targetIndexSettings); - IndexMetadata.selectCloneShard(0, sourceMetadata, IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); + IndexMetadata.selectCloneShard(0, sourceMetadata, INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); } static IndexMetadata validateResize(ClusterState state, String sourceIndex, @@ -1147,11 +1153,11 @@ public class MetadataCreateIndexService { ", all mappings are copied from the source index"); } - if (IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.exists(targetIndexSettings)) { + if (INDEX_NUMBER_OF_SHARDS_SETTING.exists(targetIndexSettings)) { // this method applies all necessary checks ie. if the target shards are less than the source shards // of if the source shards are divisible by the number of target shards IndexMetadata.getRoutingFactor(sourceMetadata.getNumberOfShards(), - IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); + INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); } return sourceMetadata; } 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 9c77d2c5640..f6478491532 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -201,8 +201,10 @@ public class MetadataUpdateSettingsService { * including updating it to null, indicating that they want to use the default value. In this case, we again * have to provide an explicit value for the setting to the default (one). */ - if (indexSettings.get(IndexMetadata.SETTING_NUMBER_OF_REPLICAS) == null) { - indexSettings.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1); + if (IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.exists(indexSettings) == false) { + indexSettings.put( + IndexMetadata.SETTING_NUMBER_OF_REPLICAS, + IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(Settings.EMPTY)); } Settings finalSettings = indexSettings.build(); indexScopedSettings.validate( @@ -228,8 +230,10 @@ public class MetadataUpdateSettingsService { * including updating it to null, indicating that they want to use the default value. In this case, we again * have to provide an explicit value for the setting to the default (one). */ - if (indexSettings.get(IndexMetadata.SETTING_NUMBER_OF_REPLICAS) == null) { - indexSettings.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1); + if (IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.exists(indexSettings) == false) { + indexSettings.put( + IndexMetadata.SETTING_NUMBER_OF_REPLICAS, + IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(Settings.EMPTY)); } Settings finalSettings = indexSettings.build(); indexScopedSettings.validate( diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index 796123d9cb6..fc92e41befd 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -406,7 +406,15 @@ public class Setting implements ToXContentObject { * @return true if the setting is present in the given settings instance, otherwise false */ public boolean exists(final Settings settings) { - return settings.keySet().contains(getKey()); + return exists(settings.keySet()); + } + + public boolean exists(final Settings.Builder builder) { + return exists(builder.keys()); + } + + private boolean exists(final Set keys) { + return keys.contains(getKey()); } /** diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexMetadataTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexMetadataTests.java index b635d4bb81e..6a2f7bd27ff 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexMetadataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexMetadataTests.java @@ -52,6 +52,8 @@ import java.util.HashMap; import java.util.Map; import java.util.Set; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; public class IndexMetadataTests extends ESTestCase { @@ -321,4 +323,50 @@ public class IndexMetadataTests extends ESTestCase { assertNotNull(meta.mappingOrDefault()); assertEquals("type", meta.mappingOrDefault().type()); } + + public void testMissingNumberOfShards() { + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> IndexMetadata.builder("test").build()); + assertThat(e.getMessage(), containsString("must specify number of shards for index [test]")); + } + + public void testNumberOfShardsIsNotZero() { + runTestNumberOfShardsIsPositive(0); + } + + public void testNumberOfShardsIsNotNegative() { + runTestNumberOfShardsIsPositive(-randomIntBetween(1, Integer.MAX_VALUE)); + } + + private void runTestNumberOfShardsIsPositive(final int numberOfShards) { + final Settings settings = + Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numberOfShards).build(); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> IndexMetadata.builder("test").settings(settings).build()); + assertThat( + e.getMessage(), + equalTo("Failed to parse value [" + numberOfShards + "] for setting [index.number_of_shards] must be >= 1")); + } + + public void testMissingNumberOfReplicas() { + final Settings settings = + Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, randomIntBetween(1, 8)).build(); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> IndexMetadata.builder("test").settings(settings).build()); + assertThat(e.getMessage(), containsString("must specify number of replicas for index [test]")); + } + + public void testNumberOfReplicasIsNonNegative() { + final int numberOfReplicas = -randomIntBetween(1, Integer.MAX_VALUE); + final Settings settings = Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, randomIntBetween(1, 8)) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numberOfReplicas) + .build(); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> IndexMetadata.builder("test").settings(settings).build()); + assertThat( + e.getMessage(), + equalTo( + "Failed to parse value [" + numberOfReplicas + "] for setting [index.number_of_replicas] must be >= 0")); + } + }