Add strong typing through Settings

jasontedor · Sep 30, 2019 · 606d374 · 606d374
1 parent abbec9b
commit 606d374
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 46 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
@@ -166,8 +166,8 @@ public void validate(final Integer value) {
             }
 
             @Override
-            public void validate(final Integer numRoutingShards, final Map<Setting<?>, Object> settings) {
-                int numShards = (int) settings.get(INDEX_NUMBER_OF_SHARDS_SETTING);
+            public void validate(final Integer numRoutingShards, final Settings settings) {
+                int numShards = INDEX_NUMBER_OF_SHARDS_SETTING.get(settings);
                 if (numRoutingShards < numShards) {
                     throw new IllegalArgumentException("index.number_of_routing_shards [" + numRoutingShards
                         + "] must be >= index.number_of_shards [" + numShards + "]");

diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java
@@ -31,7 +31,6 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;
 
 /**
  * A container to keep settings for disk thresholds up to date with cluster setting changes.
@@ -109,9 +108,9 @@ public void validate(String value) {
         }
 
         @Override
-        public void validate(final String value, final Map<Setting<?>, Object> settings) {
-            final String highWatermarkRaw = (String) settings.get(CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING);
-            final String floodStageRaw = (String) settings.get(CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING);
+        public void validate(final String value, final Settings settings) {
+            final String highWatermarkRaw = CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.get(settings);
+            final String floodStageRaw = CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.get(settings);
             doValidate(value, highWatermarkRaw, floodStageRaw);
         }
 
@@ -133,9 +132,9 @@ public void validate(final String value) {
         }
 
         @Override
-        public void validate(final String value, final Map<Setting<?>, Object> settings) {
-            final String lowWatermarkRaw = (String) settings.get(CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING);
-            final String floodStageRaw = (String) settings.get(CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING);
+        public void validate(final String value, final Settings settings) {
+            final String lowWatermarkRaw = CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.get(settings);
+            final String floodStageRaw = CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.get(settings);
             doValidate(lowWatermarkRaw, value, floodStageRaw);
         }
 
@@ -157,9 +156,9 @@ public void validate(final String value) {
         }
 
         @Override
-        public void validate(final String value, final Map<Setting<?>, Object> settings) {
-            final String lowWatermarkRaw = (String) settings.get(CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING);
-            final String highWatermarkRaw = (String) settings.get(CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING);
+        public void validate(final String value, Settings settings) {
+            final String lowWatermarkRaw = CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.get(settings);
+            final String highWatermarkRaw = CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.get(settings);
             doValidate(lowWatermarkRaw, highWatermarkRaw, value);
         }
 

diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java
@@ -433,18 +433,19 @@ private T get(Settings settings, boolean validate) {
             T parsed = parser.apply(value);
             if (validate) {
                 final Iterator<Setting<?>> it = validator.settings();
-                final Map<Setting<?>, Object> map;
+                final Settings dependentSettings;
                 if (it.hasNext()) {
-                    map = new HashMap<>();
+                    final Settings.Builder builder = Settings.builder();
                     while (it.hasNext()) {
                         final Setting<?> setting = it.next();
-                        map.put(setting, setting.get(settings, false)); // we have to disable validation or we will stack overflow
+                        builder.put(setting.getKey(), setting.getRaw(settings));
                     }
+                    dependentSettings = builder.build();
                 } else {
-                    map = Collections.emptyMap();
+                    dependentSettings = Settings.EMPTY;
                 }
                 validator.validate(parsed);
-                validator.validate(parsed, map);
+                validator.validate(parsed, dependentSettings);
             }
             return parsed;
         } catch (ElasticsearchParseException ex) {
@@ -841,8 +842,8 @@ public Map<String, T> getAsMap(Settings settings) {
 
     /**
      * Represents a validator for a setting. The {@link #validate(Object)} method is invoked early in the update setting process with the
-     * value of this setting for a fail-fast validation. Later on, the {@link #validate(Object, Map)} method is invoked with the value of
-     * this setting and a map from the settings specified by {@link #settings()}} to their values. All these values come from the same
+     * value of this setting for a fail-fast validation. Later on, the {@link #validate(Object, Settings)} method is invoked with the value
+     * of this setting and a {@link Settings} instance containing the dependent settings. All these values come from the same
      * {@link Settings} instance.
      *
      * @param <T> the type of the {@link Setting}
@@ -862,14 +863,14 @@ public interface Validator<T> {
          * accepting any value as valid as long as it passes the validation in {@link #validate(Object)}.
          *
          * @param value    the value of this setting
-         * @param settings a map from the settings specified by {@link #settings()}} to their values
+         * @param settings the dependent settings
          */
-        default void validate(T value, Map<Setting<?>, Object> settings) {
+        default void validate(T value, Settings settings) {
         }
 
         /**
          * The settings on which the validity of this setting depends. The values of the specified settings are passed to
-         * {@link #validate(Object, Map)}. By default this returns an empty iterator, indicating that this setting does not depend on any
+         * {@link #validate(Object, Settings)}. By default this returns an empty iterator, indicating that this setting does not depend on any
          * other settings.
          *
          * @return the settings on which the validity of this setting depends.

diff --git a/server/src/main/java/org/elasticsearch/index/IndexSettings.java b/server/src/main/java/org/elasticsearch/index/IndexSettings.java
@@ -38,7 +38,6 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -317,8 +316,8 @@ public void validate(final String value) {
         }
 
         @Override
-        public void validate(final String value, final Map<Setting<?>, Object> settings) {
-            final String requiredPipeline = (String) settings.get(IndexSettings.REQUIRED_PIPELINE);
+        public void validate(final String value, final Settings settings) {
+            final String requiredPipeline = DEFAULT_PIPELINE.get(settings);
             if (value.equals(IngestService.NOOP_PIPELINE_NAME) == false
                 && requiredPipeline.equals(IngestService.NOOP_PIPELINE_NAME) == false) {
                 throw new IllegalArgumentException(
@@ -342,8 +341,8 @@ public void validate(final String value) {
         }
 
         @Override
-        public void validate(final String value, final Map<Setting<?>, Object> settings) {
-            final String defaultPipeline = (String) settings.get(IndexSettings.DEFAULT_PIPELINE);
+        public void validate(final String value, final Settings settings) {
+            final String defaultPipeline = DEFAULT_PIPELINE.get(settings);
             if (value.equals(IngestService.NOOP_PIPELINE_NAME) && defaultPipeline.equals(IngestService.NOOP_PIPELINE_NAME) == false) {
                 throw new IllegalArgumentException(
                     "index has a required pipeline [" + value + "] and a default pipeline [" + defaultPipeline + "]");

diff --git a/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java b/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java
@@ -31,7 +31,6 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.ThreadFactory;
 
@@ -84,10 +83,10 @@ public void validate(final Integer value) {
                 }
 
                 @Override
-                public void validate(final Integer value, final Map<Setting<?>, Object> settings) {
-                    if (value > (int) settings.get(tempMaxQueueSizeSetting)) {
+                public void validate(final Integer value, final Settings settings) {
+                    if (value > tempMaxQueueSizeSetting.get(settings)) {
                         throw new IllegalArgumentException("Failed to parse value [" + value + "] for setting [" + minSizeKey
-                            + "] must be <= " + settings.get(tempMaxQueueSizeSetting));
+                            + "] must be <= " + tempMaxQueueSizeSetting.get(settings));
                     }
                 }
 
@@ -111,10 +110,10 @@ public void validate(Integer value) {
                     }
 
                     @Override
-                    public void validate(final Integer value, final Map<Setting<?>, Object> settings) {
-                        if (value < (int) settings.get(tempMinQueueSizeSetting)) {
+                    public void validate(final Integer value, final Settings settings) {
+                        if (value < tempMinQueueSizeSetting.get(settings)) {
                             throw new IllegalArgumentException("Failed to parse value [" + value + "] for setting [" + minSizeKey
-                                + "] must be >= " + settings.get(tempMinQueueSizeSetting));
+                                + "] must be >= " + tempMinQueueSizeSetting.get(settings));
                         }
                     }
 

diff --git a/...r/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/...r/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java
@@ -32,7 +32,6 @@
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
@@ -515,7 +514,7 @@ public void validate(final String value) {
                 }
 
                 @Override
-                public void validate(final String value, final Map<Setting<?>, Object> settings) {
+                public void validate(final String value, final Settings settings) {
 
                 }
 
@@ -532,7 +531,7 @@ public void validate(final String value) {
                 }
 
                 @Override
-                public void validate(final String value, final Map<Setting<?>, Object> settings) {
+                public void validate(final String value, final Settings settings) {
                     throw new IllegalArgumentException("Invalid with dependencies setting");
                 }
 
@@ -547,9 +546,9 @@ public void validate(final Integer value) {
         }
 
         @Override
-        public void validate(final Integer low, final Map<Setting<?>, Object> settings) {
-            if (settings.containsKey(SETTING_FOO_HIGH) && low > (int) settings.get(SETTING_FOO_HIGH)) {
-                throw new IllegalArgumentException("[low]=" + low + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH));
+        public void validate(final Integer low, final Settings settings) {
+            if (SETTING_FOO_HIGH.exists(settings) && low > SETTING_FOO_HIGH.get(settings)) {
+                throw new IllegalArgumentException("[low]=" + low + " is higher than [high]=" + SETTING_FOO_HIGH.get(settings));
             }
         }
 
@@ -569,9 +568,9 @@ public void validate(final Integer value) {
         }
 
         @Override
-        public void validate(final Integer high, final Map<Setting<?>, Object> settings) {
-            if (settings.containsKey(SETTING_FOO_LOW) && high < (int) settings.get(SETTING_FOO_LOW)) {
-                throw new IllegalArgumentException("[high]=" + high + " is lower than [low]=" + settings.get(SETTING_FOO_LOW));
+        public void validate(final Integer high, final Settings settings) {
+            if ((SETTING_FOO_LOW.exists(settings)) && high < SETTING_FOO_LOW.get(settings)) {
+                throw new IllegalArgumentException("[high]=" + high + " is lower than [low]=" + SETTING_FOO_LOW.get(settings));
             }
         }
 

diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java
@@ -214,12 +214,12 @@ public void validate(final String value) {
         }
 
         @Override
-        public void validate(final String value, final Map<Setting<?>, Object> settings) {
+        public void validate(final String value, final Settings settings) {
             invokedWithDependencies = true;
             assertTrue(settings.keySet().contains(BAZ_QUX_SETTING));
-            assertThat(settings.get(BAZ_QUX_SETTING), equalTo("baz.qux value"));
+            assertThat(BAZ_QUX_SETTING.get(settings), equalTo("baz.qux value"));
             assertTrue(settings.keySet().contains(QUUX_QUUZ_SETTING));
-            assertThat(settings.get(QUUX_QUUZ_SETTING), equalTo("quux.quuz value"));
+            assertThat(QUUX_QUUZ_SETTING.get(settings), equalTo("quux.quuz value"));
         }
 
         @Override