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.

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

@@ -1080,29 +1080,26 @@ public class IndexMetadata implements Diffable<IndexMetadata>, ToXContentFragmen
             ImmutableOpenMap.Builder<String, AliasMetadata> 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<Set<String>> filledInSyncAllocationIds = ImmutableOpenIntMap.builder();
             for (int i = 0; i < numberOfShards; i++) {

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

@@ -99,6 +99,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;
@@ -762,11 +764,11 @@ 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) {
-            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 @@ 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) {
@@ -1034,7 +1036,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);
 
@@ -1100,8 +1102,8 @@ public class MetadataCreateIndexService {
                                             Set<String> 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 @@ public class MetadataCreateIndexService {
                                    Set<String> 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<String> 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 @@ 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;
     }

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(

server/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -405,8 +405,15 @@ public class Setting<T> implements ToXContentObject {
      * @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<String> keys, final SecureSettings secureSettings) {
+        return keys.contains(getKey()) &&
             (secureSettings == null || secureSettings.getSettingNames().contains(getKey()) == false);
     }
 

server/src/test/java/org/elasticsearch/cluster/metadata/IndexMetadataTests.java

@@ -50,6 +50,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 {
@@ -284,4 +286,49 @@ public class IndexMetadataTests extends ESTestCase {
         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"));
+    }
+
 }