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 bee1589170cd6..c3463920d138c 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java @@ -1080,29 +1080,26 @@ public IndexMetadata build() { ImmutableOpenMap.Builder tmpAliases = aliases; Settings tmpSettings = settings; - 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 5724b76af17b4..d0cb5e941baf5 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -99,6 +99,8 @@ 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; @@ -762,11 +764,11 @@ static Settings aggregateIndexSettings(ClusterState currentState, CreateIndexClu 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) { - indexSettingsBuilder.put(SETTING_NUMBER_OF_SHARDS, settings.getAsInt(SETTING_NUMBER_OF_SHARDS, 1)); + if (INDEX_NUMBER_OF_SHARDS_SETTING.exists(indexSettingsBuilder) == false) { + indexSettingsBuilder.put(SETTING_NUMBER_OF_SHARDS, INDEX_NUMBER_OF_SHARDS_SETTING.get(settings)); } - 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)); @@ -811,7 +813,7 @@ static Settings aggregateIndexSettings(ClusterState currentState, CreateIndexClu * 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) { @@ -1034,7 +1036,7 @@ public void validateIndexSettings(String indexName, final Settings settings, fin * @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); @@ -1100,8 +1102,8 @@ static List validateShrinkIndex(ClusterState state, String sourceIndex, 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"); @@ -1133,14 +1135,14 @@ static void validateSplitIndex(ClusterState state, String sourceIndex, Set targetIndexMappingsTypes, String targetIndexName, Settings targetIndexSettings) { IndexMetadata sourceMetadata = validateResize(state, sourceIndex, targetIndexMappingsTypes, targetIndexName, targetIndexSettings); - IndexMetadata.selectSplitShard(0, sourceMetadata, IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); + IndexMetadata.selectSplitShard(0, sourceMetadata, INDEX_NUMBER_OF_SHARDS_SETTING.get(targetIndexSettings)); } 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, @@ -1163,11 +1165,11 @@ static IndexMetadata validateResize(ClusterState state, String sourceIndex, ", 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 9c77d2c564073..f647849153292 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 ClusterState execute(ClusterState currentState) { * 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 ClusterState execute(ClusterState currentState) { * 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 0680dd7c2ccfd..614a72f3a0d90 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -405,8 +405,15 @@ public T getDefault(Settings settings) { * @return true if the setting is present in the given settings instance, otherwise false */ public boolean exists(final Settings settings) { - SecureSettings secureSettings = settings.getSecureSettings(); - return settings.keySet().contains(getKey()) && + return exists(settings.keySet(), settings.getSecureSettings()); + } + + public boolean exists(final Settings.Builder builder) { + return exists(builder.keys(), builder.getSecureSettings()); + } + + private boolean exists(final Set keys, final SecureSettings secureSettings) { + return keys.contains(getKey()) && (secureSettings == null || secureSettings.getSettingNames().contains(getKey()) == false); } 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 d55a35446df8d..4f3e0fad37b01 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexMetadataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexMetadataTests.java @@ -50,6 +50,8 @@ 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 { @@ -284,4 +286,49 @@ public void testNumberOfRoutingShards() { assertEquals("the number of source shards [2] must be a factor of [3]", iae.getMessage()); } + 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")); + } + }