From 8852b05bee09a802906ad1620bb1846c443984ed Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 1 Oct 2018 15:27:32 +0200 Subject: [PATCH 01/28] Validate current setting against all target settings (#28309) --- .../settings/AbstractScopedSettings.java | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index e87b3757e6b28..1375c3d144915 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -716,7 +716,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin } else if (get(key) == null) { throw new IllegalArgumentException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { - validate(key, toApply, false); // we might not have a full picture here do to a dependency validation + validate(key, toApply, target.build()); settingsBuilder.copy(key, toApply); updates.copy(key, toApply); changed = true; @@ -733,6 +733,26 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin return changed; } + private void validate(String key, Settings toApply, Settings target) { + Settings.Builder toValidate = Settings.builder(); + // Existing settings reused to have to bigger picture, + // this help to validate runtime dependencies + // (e.g. disk watermarks "low", "high" and "flood_stage"). + // Add only valid target settings to "toValidate" settings + target.keySet().forEach(targetKey -> { + try { + validate(targetKey, target, false); + toValidate.copy(targetKey, target); + } catch (Exception ignored) { + // Don't add invalid settings for validation + } + }); + // Put last to override existing setting + toValidate.copy(key, toApply); + // We might not have a full picture here do to a dependency validation + validate(toValidate.build(), false); + } + private static boolean applyDeletes(Set deletes, Settings.Builder builder, Predicate canRemove) { boolean changed = false; for (String entry : deletes) { From 5a84c182dc9b81d78a15df2acd04daf40fc7ec48 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Tue, 2 Oct 2018 18:34:02 +0200 Subject: [PATCH 02/28] Fix misspelled comment (#28309) --- .../common/settings/AbstractScopedSettings.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 1375c3d144915..5c65a9f5c4ee6 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -735,10 +735,10 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin private void validate(String key, Settings toApply, Settings target) { Settings.Builder toValidate = Settings.builder(); - // Existing settings reused to have to bigger picture, - // this help to validate runtime dependencies - // (e.g. disk watermarks "low", "high" and "flood_stage"). - // Add only valid target settings to "toValidate" settings + // Reuse target settings to have a bigger picture, + // this helps to validate runtime dependencies + // (e.g. "low", "high" and "flood_stage" disk watermarks). + // Only valid target settings are kept. target.keySet().forEach(targetKey -> { try { validate(targetKey, target, false); @@ -747,7 +747,7 @@ private void validate(String key, Settings toApply, Settings target) { // Don't add invalid settings for validation } }); - // Put last to override existing setting + // Put last to override existing target setting toValidate.copy(key, toApply); // We might not have a full picture here do to a dependency validation validate(toValidate.build(), false); From 71cfe1a874d550056d8b7facbc9b7acb04b92779 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Thu, 4 Oct 2018 14:46:24 +0200 Subject: [PATCH 03/28] Test sequence of updates (#28309) --- .../allocation/DiskThresholdSettingsTests.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java index 342fcea7ddef1..89c7b6738178d 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java @@ -203,4 +203,20 @@ public void testInvalidHighDiskThreshold() { assertThat(cause, hasToString(containsString("low disk watermark [85%] more than high disk watermark [75%]"))); } + public void testSequenceOfUpdates() { + Settings settings = Settings.EMPTY; + + final ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + new DiskThresholdSettings(settings, clusterSettings); // this has the effect of registering the settings updater + + settings = clusterSettings.applySettings(Settings.builder() + .put(settings) + .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.getKey(), "99%") + .build()); + settings = clusterSettings.applySettings(Settings.builder() + .put(settings) + .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey(), "97%") + .build()); + } + } From f77d1e70089f5af03e7f47c54d6ba2b75959dc51 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Thu, 4 Oct 2018 16:01:41 +0200 Subject: [PATCH 04/28] Add setting to apply to validation collection (#28309) --- .../settings/AbstractScopedSettings.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 5c65a9f5c4ee6..cc5dcc6aed8e1 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -735,22 +735,21 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin private void validate(String key, Settings toApply, Settings target) { Settings.Builder toValidate = Settings.builder(); - // Reuse target settings to have a bigger picture, - // this helps to validate runtime dependencies - // (e.g. "low", "high" and "flood_stage" disk watermarks). - // Only valid target settings are kept. - target.keySet().forEach(targetKey -> { + validateAndCopy(target, toValidate); + validateAndCopy(toApply, toValidate); + toValidate.copy(key, toApply); + validate(toValidate.build(), false); + } + + private void validateAndCopy(Settings toCopy, Settings.Builder toValidate) { + toCopy.keySet().forEach(key -> { try { - validate(targetKey, target, false); - toValidate.copy(targetKey, target); + validate(key, toCopy, false); + toValidate.copy(key, toCopy); } catch (Exception ignored) { // Don't add invalid settings for validation } }); - // Put last to override existing target setting - toValidate.copy(key, toApply); - // We might not have a full picture here do to a dependency validation - validate(toValidate.build(), false); } private static boolean applyDeletes(Set deletes, Settings.Builder builder, Predicate canRemove) { From b0dd2ded709b5e7fb33b7df02e96f7ca1586d0b1 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Thu, 4 Oct 2018 22:35:02 +0200 Subject: [PATCH 05/28] Test update of validation dependant settings (#28309) --- .../settings/AbstractScopedSettings.java | 22 +++--- .../common/settings/ScopedSettingsTests.java | 73 +++++++++++++++++++ 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index cc5dcc6aed8e1..414b7f5f459b7 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -58,6 +58,7 @@ public abstract class AbstractScopedSettings extends AbstractComponent { private static final Pattern KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])*[-\\w]+$"); private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$"); private static final Pattern AFFIX_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+[*](?:[.][-\\w]+)+$"); + private static final Predicate NOT_WILDCARD_SETTING = s -> !s.endsWith("*"); protected AbstractScopedSettings( final Settings settings, @@ -716,7 +717,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin } else if (get(key) == null) { throw new IllegalArgumentException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { - validate(key, toApply, target.build()); + validate(toApply, target.build()); settingsBuilder.copy(key, toApply); updates.copy(key, toApply); changed = true; @@ -733,23 +734,22 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin return changed; } - private void validate(String key, Settings toApply, Settings target) { - Settings.Builder toValidate = Settings.builder(); - validateAndCopy(target, toValidate); - validateAndCopy(toApply, toValidate); - toValidate.copy(key, toApply); - validate(toValidate.build(), false); + private void validate(Settings toApply, Settings target) { + Settings toValidate = copyValidSettings(target).put(toApply.filter(NOT_WILDCARD_SETTING)).build(); + validate(toValidate, false); } - private void validateAndCopy(Settings toCopy, Settings.Builder toValidate) { - toCopy.keySet().forEach(key -> { + private Settings.Builder copyValidSettings(Settings settings) { + Settings.Builder validSettings = Settings.builder(); + settings.keySet().stream().filter(NOT_WILDCARD_SETTING).forEach(key -> { try { - validate(key, toCopy, false); - toValidate.copy(key, toCopy); + validate(key, settings, false); + validSettings.copy(key, settings); } catch (Exception ignored) { // Don't add invalid settings for validation } }); + return validSettings; } private static boolean applyDeletes(Set deletes, Settings.Builder builder, Predicate canRemove) { diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 10c58c562ad5b..42026b578da73 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -59,6 +59,40 @@ public class ScopedSettingsTests extends ESTestCase { + private static class FooLowSettingValidator implements Setting.Validator { + @Override + public void validate(Integer value, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_HIGH) && value > settings.get(SETTING_FOO_HIGH)) { + throw new IllegalArgumentException("[low]=" + value + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH)); + } + } + + @Override + public Iterator> settings() { + return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); + } + } + + private static class FooHighSettingValidator implements Setting.Validator { + @Override + public void validate(Integer value, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_LOW) && value < settings.get(SETTING_FOO_LOW)) { + throw new IllegalArgumentException("[high]=" + value + " is lower than [low]=" + settings.get(SETTING_FOO_LOW)); + } + } + + @Override + public Iterator> settings() { + return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); + } + } + + private static final Setting SETTING_FOO_LOW = new Setting("foo.low", "10", + Integer::valueOf, new FooLowSettingValidator(), Setting.Property.NodeScope); + private static final Setting SETTING_FOO_HIGH = new Setting("foo.high", "100", + Integer::valueOf, new FooHighSettingValidator(), Setting.Property.NodeScope); + + public void testResetSetting() { Setting dynamicSetting = Setting.intSetting("some.dyn.setting", 1, Property.Dynamic, Property.NodeScope); Setting staticSetting = Setting.intSetting("some.static.setting", 1, Property.NodeScope); @@ -1221,4 +1255,43 @@ public List getListValue(final List value) { equalTo(oldSetting.get(settings).stream().map(s -> "new." + s).collect(Collectors.toList()))); } + public void testUpdateOfValidationDependantSettings() { + ClusterSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH))); + + Settings.Builder target = Settings.builder(); + Settings.Builder updates; + boolean updated; + + updates = Settings.builder(); + updated = service.updateSettings( + Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), + target, + updates, + "transient" + ); + assertTrue(updated); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + + updates = Settings.builder(); + updated = service.updateSettings( + Settings.builder().put(SETTING_FOO_HIGH.getKey(), 40).build(), + target, + updates, + "transient" + ); + assertTrue(updated); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); + assertNull(updates.get(SETTING_FOO_LOW.getKey())); + assertThat(updates.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); + + Exception exception = expectThrows(IllegalArgumentException.class, () -> service.updateSettings( + Settings.builder().put(SETTING_FOO_HIGH.getKey(), 10).build(), + target, + Settings.builder(), + "transient" + )); + assertThat(exception.getMessage(), equalTo("[high]=10 is lower than [low]=20")); + } } From 177f560e4f971e4579c647379cfcc92b80b5c2f3 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Fri, 5 Oct 2018 08:53:37 +0200 Subject: [PATCH 06/28] Fix comment (#28309) --- .../elasticsearch/common/settings/AbstractScopedSettings.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 414b7f5f459b7..b6286219310cf 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -746,7 +746,7 @@ private Settings.Builder copyValidSettings(Settings settings) { validate(key, settings, false); validSettings.copy(key, settings); } catch (Exception ignored) { - // Don't add invalid settings for validation + // ignore invalid settings } }); return validSettings; From 94ac1d1b6db0f942a5deb026c2defb3b3be8778f Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Fri, 5 Oct 2018 11:04:28 +0200 Subject: [PATCH 07/28] Improve variable naming to ease readability (#28309) --- .../common/settings/AbstractScopedSettings.java | 2 +- .../common/settings/ScopedSettingsTests.java | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index b6286219310cf..45d9d0bbc250f 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -58,7 +58,7 @@ public abstract class AbstractScopedSettings extends AbstractComponent { private static final Pattern KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])*[-\\w]+$"); private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$"); private static final Pattern AFFIX_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+[*](?:[.][-\\w]+)+$"); - private static final Predicate NOT_WILDCARD_SETTING = s -> !s.endsWith("*"); + private static final Predicate NOT_WILDCARD_SETTING = key -> !key.endsWith("*"); protected AbstractScopedSettings( final Settings settings, diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 42026b578da73..5686e6c1f582d 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -61,9 +61,9 @@ public class ScopedSettingsTests extends ESTestCase { private static class FooLowSettingValidator implements Setting.Validator { @Override - public void validate(Integer value, Map, Integer> settings) { - if (settings.containsKey(SETTING_FOO_HIGH) && value > settings.get(SETTING_FOO_HIGH)) { - throw new IllegalArgumentException("[low]=" + value + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH)); + public void validate(Integer low, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_HIGH) && low > settings.get(SETTING_FOO_HIGH)) { + throw new IllegalArgumentException("[low]=" + low + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH)); } } @@ -75,9 +75,9 @@ public Iterator> settings() { private static class FooHighSettingValidator implements Setting.Validator { @Override - public void validate(Integer value, Map, Integer> settings) { - if (settings.containsKey(SETTING_FOO_LOW) && value < settings.get(SETTING_FOO_LOW)) { - throw new IllegalArgumentException("[high]=" + value + " is lower than [low]=" + settings.get(SETTING_FOO_LOW)); + public void validate(Integer high, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_LOW) && high < settings.get(SETTING_FOO_LOW)) { + throw new IllegalArgumentException("[high]=" + high + " is lower than [low]=" + settings.get(SETTING_FOO_LOW)); } } From 68044ba4cf59b709c4879fd56da1047ce6912c2f Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Fri, 5 Oct 2018 12:48:01 +0200 Subject: [PATCH 08/28] Don't update settings when no value has changed (#28309) --- .../settings/AbstractScopedSettings.java | 25 +++++++++++++++---- .../common/settings/ScopedSettingsTests.java | 12 +++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 45d9d0bbc250f..31dbe7243e9ad 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -34,6 +34,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.BiConsumer; @@ -58,7 +59,8 @@ public abstract class AbstractScopedSettings extends AbstractComponent { private static final Pattern KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])*[-\\w]+$"); private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$"); private static final Pattern AFFIX_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+[*](?:[.][-\\w]+)+$"); - private static final Predicate NOT_WILDCARD_SETTING = key -> !key.endsWith("*"); + private static final Predicate WILDCARD_SETTING = key -> key.endsWith("*"); + private static final Predicate NOT_WILDCARD_SETTING = key -> !WILDCARD_SETTING.test(key); protected AbstractScopedSettings( final Settings settings, @@ -717,10 +719,12 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin } else if (get(key) == null) { throw new IllegalArgumentException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { - validate(toApply, target.build()); - settingsBuilder.copy(key, toApply); - updates.copy(key, toApply); - changed = true; + changed = hasChanged(toApply, target); + if (changed) { + validate(toApply, target.build()); + settingsBuilder.copy(key, toApply); + updates.copy(key, toApply); + } } else { if (isFinalSetting(key)) { throw new IllegalArgumentException("final " + type + " setting [" + key + "], not updateable"); @@ -752,6 +756,17 @@ private Settings.Builder copyValidSettings(Settings settings) { return validSettings; } + private boolean hasChanged(Settings toApply, Settings.Builder target) { + boolean changed = toApply.keySet().stream().anyMatch(k -> k.endsWith("*")); + if (!changed) { + changed = target.build().keySet().stream().anyMatch(k -> k.endsWith("*")); + } + if (!changed) { + changed = toApply.keySet().stream().anyMatch(k -> !Objects.equals(toApply.get(k), target.get(k))); + } + return changed; + } + private static boolean applyDeletes(Set deletes, Settings.Builder builder, Predicate canRemove) { boolean changed = false; for (String entry : deletes) { diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 5686e6c1f582d..872089f29e143 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -54,6 +54,7 @@ import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.startsWith; import static org.hamcrest.Matchers.arrayWithSize; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.sameInstance; @@ -1273,6 +1274,17 @@ public void testUpdateOfValidationDependantSettings() { assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + updates = Settings.builder(); + updated = service.updateSettings( + Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), + target, + updates, + "transient" + ); + assertFalse(updated); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + assertThat(updates.keys(), empty()); + updates = Settings.builder(); updated = service.updateSettings( Settings.builder().put(SETTING_FOO_HIGH.getKey(), 40).build(), From 8e138d9ef3ee1970456d0dbc7436d1a6fabc6168 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 8 Oct 2018 13:20:51 +0200 Subject: [PATCH 09/28] Move validation API with other ones (#28309) --- .../settings/AbstractScopedSettings.java | 62 ++++++++----------- 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 31dbe7243e9ad..f77f3bde40b36 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -34,7 +34,6 @@ import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.BiConsumer; @@ -355,6 +354,28 @@ public synchronized void addSettingsUpdateConsumer(Setting setting, Consu addSettingsUpdateConsumer(setting, consumer, (s) -> {}); } + /** + * Validates that all settings are registered and valid against other valid settings. + * + * @param settings the settings to validates + * @param others the other settings to validate against + * @param validateDependencies true if dependent settings should be validated + * @see Setting#getSettingsDependencies(String) + */ + public final void validate(Settings settings, Settings others, boolean validateDependencies) { + final Settings.Builder validSettings = Settings.builder(); + others.keySet().stream().filter(NOT_WILDCARD_SETTING).forEach(key -> { + try { + validate(key, others, validateDependencies); + validSettings.copy(key, others); + } catch (Exception ignored) { + // ignore invalid settings + } + }); + final Settings toValidate = validSettings.put(settings.filter(NOT_WILDCARD_SETTING)).build(); + validate(toValidate, validateDependencies); + } + /** * Validates that all settings are registered and valid. * @@ -719,12 +740,10 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin } else if (get(key) == null) { throw new IllegalArgumentException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { - changed = hasChanged(toApply, target); - if (changed) { - validate(toApply, target.build()); - settingsBuilder.copy(key, toApply); - updates.copy(key, toApply); - } + validate(toApply, target.build(), false); + settingsBuilder.copy(key, toApply); + updates.copy(key, toApply); + changed = true; } else { if (isFinalSetting(key)) { throw new IllegalArgumentException("final " + type + " setting [" + key + "], not updateable"); @@ -738,35 +757,6 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin return changed; } - private void validate(Settings toApply, Settings target) { - Settings toValidate = copyValidSettings(target).put(toApply.filter(NOT_WILDCARD_SETTING)).build(); - validate(toValidate, false); - } - - private Settings.Builder copyValidSettings(Settings settings) { - Settings.Builder validSettings = Settings.builder(); - settings.keySet().stream().filter(NOT_WILDCARD_SETTING).forEach(key -> { - try { - validate(key, settings, false); - validSettings.copy(key, settings); - } catch (Exception ignored) { - // ignore invalid settings - } - }); - return validSettings; - } - - private boolean hasChanged(Settings toApply, Settings.Builder target) { - boolean changed = toApply.keySet().stream().anyMatch(k -> k.endsWith("*")); - if (!changed) { - changed = target.build().keySet().stream().anyMatch(k -> k.endsWith("*")); - } - if (!changed) { - changed = toApply.keySet().stream().anyMatch(k -> !Objects.equals(toApply.get(k), target.get(k))); - } - return changed; - } - private static boolean applyDeletes(Set deletes, Settings.Builder builder, Predicate canRemove) { boolean changed = false; for (String entry : deletes) { From 6539bd0fc362e093e4bf0823ecd6e0e12e9525e4 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 8 Oct 2018 13:38:21 +0200 Subject: [PATCH 10/28] Add a test case when settings are below "low" default value (#28309) --- .../common/settings/ScopedSettingsTests.java | 158 +++++++++--------- 1 file changed, 78 insertions(+), 80 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 872089f29e143..0387fb13331c4 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -54,46 +54,11 @@ import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.startsWith; import static org.hamcrest.Matchers.arrayWithSize; -import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.sameInstance; public class ScopedSettingsTests extends ESTestCase { - private static class FooLowSettingValidator implements Setting.Validator { - @Override - public void validate(Integer low, Map, Integer> settings) { - if (settings.containsKey(SETTING_FOO_HIGH) && low > settings.get(SETTING_FOO_HIGH)) { - throw new IllegalArgumentException("[low]=" + low + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH)); - } - } - - @Override - public Iterator> settings() { - return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); - } - } - - private static class FooHighSettingValidator implements Setting.Validator { - @Override - public void validate(Integer high, Map, Integer> settings) { - if (settings.containsKey(SETTING_FOO_LOW) && high < settings.get(SETTING_FOO_LOW)) { - throw new IllegalArgumentException("[high]=" + high + " is lower than [low]=" + settings.get(SETTING_FOO_LOW)); - } - } - - @Override - public Iterator> settings() { - return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); - } - } - - private static final Setting SETTING_FOO_LOW = new Setting("foo.low", "10", - Integer::valueOf, new FooLowSettingValidator(), Setting.Property.NodeScope); - private static final Setting SETTING_FOO_HIGH = new Setting("foo.high", "100", - Integer::valueOf, new FooHighSettingValidator(), Setting.Property.NodeScope); - - public void testResetSetting() { Setting dynamicSetting = Setting.intSetting("some.dyn.setting", 1, Property.Dynamic, Property.NodeScope); Setting staticSetting = Setting.intSetting("some.static.setting", 1, Property.NodeScope); @@ -1256,54 +1221,87 @@ public List getListValue(final List value) { equalTo(oldSetting.get(settings).stream().map(s -> "new." + s).collect(Collectors.toList()))); } + private static class FooLowSettingValidator implements Setting.Validator { + @Override + public void validate(Integer low, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_HIGH) && low > settings.get(SETTING_FOO_HIGH)) { + throw new IllegalArgumentException("[low]=" + low + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH)); + } + } + + @Override + public Iterator> settings() { + return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); + } + } + + private static class FooHighSettingValidator implements Setting.Validator { + @Override + public void validate(Integer high, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_LOW) && high < settings.get(SETTING_FOO_LOW)) { + throw new IllegalArgumentException("[high]=" + high + " is lower than [low]=" + settings.get(SETTING_FOO_LOW)); + } + } + + @Override + public Iterator> settings() { + return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); + } + } + + private static final Setting SETTING_FOO_LOW = new Setting<>("foo.low", "10", + Integer::valueOf, new FooLowSettingValidator(), Setting.Property.NodeScope); + private static final Setting SETTING_FOO_HIGH = new Setting<>("foo.high", "100", + Integer::valueOf, new FooHighSettingValidator(), Setting.Property.NodeScope); + public void testUpdateOfValidationDependantSettings() { ClusterSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH))); Settings.Builder target = Settings.builder(); - Settings.Builder updates; - boolean updated; - - updates = Settings.builder(); - updated = service.updateSettings( - Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), - target, - updates, - "transient" - ); - assertTrue(updated); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - - updates = Settings.builder(); - updated = service.updateSettings( - Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), - target, - updates, - "transient" - ); - assertFalse(updated); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - assertThat(updates.keys(), empty()); - - updates = Settings.builder(); - updated = service.updateSettings( - Settings.builder().put(SETTING_FOO_HIGH.getKey(), 40).build(), - target, - updates, - "transient" - ); - assertTrue(updated); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); - assertNull(updates.get(SETTING_FOO_LOW.getKey())); - assertThat(updates.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); - - Exception exception = expectThrows(IllegalArgumentException.class, () -> service.updateSettings( - Settings.builder().put(SETTING_FOO_HIGH.getKey(), 10).build(), - target, - Settings.builder(), - "transient" - )); - assertThat(exception.getMessage(), equalTo("[high]=10 is lower than [low]=20")); + + { + final Settings.Builder updates = Settings.builder(); + assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), target, updates, "transient")); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + } + + { + final Settings.Builder updates = Settings.builder(); + assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), target, updates, "transient")); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + } + + { + final Settings.Builder updates = Settings.builder(); + assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_HIGH.getKey(), 40).build(), target, updates, "transient")); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); + assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); + assertNull(updates.get(SETTING_FOO_LOW.getKey())); + assertThat(updates.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); + } + + { + final Settings.Builder updates = Settings.builder(); + assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_LOW.getKey(), 5).build(), target, updates, "transient")); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("5")); + assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); + assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("5")); + assertNull(updates.get(SETTING_FOO_HIGH.getKey())); + } + + { + final Settings.Builder updates = Settings.builder(); + assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_HIGH.getKey(), 8).build(), target, updates, "transient")); + assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("5")); + assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("8")); + assertNull(updates.get(SETTING_FOO_LOW.getKey())); + assertThat(updates.get(SETTING_FOO_HIGH.getKey()), equalTo("8")); + } + + Exception exception = expectThrows(IllegalArgumentException.class, () -> + service.updateSettings(Settings.builder().put(SETTING_FOO_HIGH.getKey(), 2).build(), target, Settings.builder(), "transient")); + assertThat(exception.getMessage(), equalTo("[high]=2 is lower than [low]=5")); } } From df341e9301e4959249f1d8164b021653cae676f2 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 8 Oct 2018 13:48:49 +0200 Subject: [PATCH 11/28] Rework "testSequenceOfUpdates" with "updateSettings" API (#28309) --- .../DiskThresholdSettingsTests.java | 55 +++++++++++++++---- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java index 89c7b6738178d..d9e157187d581 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java @@ -26,6 +26,7 @@ import java.util.Locale; +import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; @@ -204,19 +205,49 @@ public void testInvalidHighDiskThreshold() { } public void testSequenceOfUpdates() { - Settings settings = Settings.EMPTY; + final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + new DiskThresholdSettings(Settings.EMPTY, clusterSettings); // this has the effect of registering the settings updater - final ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); - new DiskThresholdSettings(settings, clusterSettings); // this has the effect of registering the settings updater - - settings = clusterSettings.applySettings(Settings.builder() - .put(settings) - .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.getKey(), "99%") - .build()); - settings = clusterSettings.applySettings(Settings.builder() - .put(settings) - .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey(), "97%") - .build()); + final Settings.Builder target = Settings.builder(); + + { + final Settings settings = Settings.builder() + .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.getKey(), "99%") + .build(); + final Settings.Builder updates = Settings.builder(); + assertTrue(clusterSettings.updateSettings(settings, target, updates, "transient")); + assertNull(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey())); + assertNull(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey())); + assertThat(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.getKey()), + equalTo("99%")); + } + + { + final Settings settings = Settings.builder() + .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey(), "97%") + .build(); + final Settings.Builder updates = Settings.builder(); + assertTrue(clusterSettings.updateSettings(settings, target, updates, "transient")); + assertNull(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey())); + assertThat(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey()), + equalTo("97%")); + assertThat(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.getKey()), + equalTo("99%")); + } + + { + final Settings settings = Settings.builder() + .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey(), "95%") + .build(); + final Settings.Builder updates = Settings.builder(); + assertTrue(clusterSettings.updateSettings(settings, target, updates, "transient")); + assertThat(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey()), + equalTo("95%")); + assertThat(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey()), + equalTo("97%")); + assertThat(target.get(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.getKey()), + equalTo("99%")); + } } } From c44b2a32bc15cfa29c69d188be931beaabf01715 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 8 Oct 2018 17:19:33 +0200 Subject: [PATCH 12/28] Filter target settings eagerly to avoid unnecessary validations (#28309) --- .../common/settings/AbstractScopedSettings.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index f77f3bde40b36..8671a6c80631d 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -364,14 +364,16 @@ public synchronized void addSettingsUpdateConsumer(Setting setting, Consu */ public final void validate(Settings settings, Settings others, boolean validateDependencies) { final Settings.Builder validSettings = Settings.builder(); - others.keySet().stream().filter(NOT_WILDCARD_SETTING).forEach(key -> { - try { - validate(key, others, validateDependencies); - validSettings.copy(key, others); - } catch (Exception ignored) { - // ignore invalid settings + for (String key : others.keySet()) { + if (NOT_WILDCARD_SETTING.test(key) && settings.keySet().contains(key) == false) { + try { + validate(key, others, validateDependencies); + validSettings.copy(key, others); + } catch (Exception ignored) { + // ignore invalid settings + } } - }); + } final Settings toValidate = validSettings.put(settings.filter(NOT_WILDCARD_SETTING)).build(); validate(toValidate, validateDependencies); } From 3e67ca49f85c246b42a7d1d6ad5bad9d2a9886d8 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 8 Oct 2018 23:11:58 +0200 Subject: [PATCH 13/28] Split settings validation with and without dependencies (#28309) --- .../ExampleCustomSettingsConfig.java | 2 +- .../cluster/metadata/IndexMetaData.java | 10 +- .../allocation/DiskThresholdSettings.java | 12 ++ .../decider/FilterAllocationDecider.java | 12 +- .../settings/AbstractScopedSettings.java | 28 +-- .../common/settings/Setting.java | 30 ++- .../AutoQueueAdjustingExecutorBuilder.java | 8 + .../settings/SettingsUpdaterTests.java | 106 ++++++++++- .../common/settings/ScopedSettingsTests.java | 174 ------------------ .../common/settings/SettingTests.java | 4 + .../xpack/core/XPackSettings.java | 2 +- .../xpack/monitoring/exporter/Exporter.java | 2 +- .../watcher/common/http/HttpSettings.java | 2 +- 13 files changed, 164 insertions(+), 228 deletions(-) diff --git a/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java b/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java index fafe3615f639d..9beb709e2fb84 100644 --- a/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java +++ b/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java @@ -49,7 +49,7 @@ public class ExampleCustomSettingsConfig { /** * A string setting that can be dynamically updated and that is validated by some logic */ - static final Setting VALIDATED_SETTING = Setting.simpleString("custom.validated", (value, settings) -> { + static final Setting VALIDATED_SETTING = Setting.simpleStringWithValidator("custom.validated", (value) -> { if (value != null && value.contains("forbidden")) { throw new IllegalArgumentException("Setting must not contain [forbidden]"); } 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 e764c16f9108b..6015b665e5fb0 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java @@ -145,6 +145,10 @@ static Setting buildNumberOfShardsSetting() { public static final Setting INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING = Setting.intSetting("index.number_of_routing_shards", INDEX_NUMBER_OF_SHARDS_SETTING, 1, new Setting.Validator() { + @Override + public void validate(Integer value) { + } + @Override public void validate(Integer numRoutingShards, Map, Integer> settings) { Integer numShards = settings.get(INDEX_NUMBER_OF_SHARDS_SETTING); @@ -212,13 +216,13 @@ public Iterator> settings() { public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude"; public static final Setting.AffixSetting INDEX_ROUTING_REQUIRE_GROUP_SETTING = Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.simpleStringWithValidator(key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INCLUDE_GROUP_SETTING = Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.simpleStringWithValidator(key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_EXCLUDE_GROUP_SETTING = Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.simpleStringWithValidator(key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INITIAL_RECOVERY_GROUP_SETTING = Setting.prefixKeySetting("index.routing.allocation.initial_recovery.", key -> Setting.simpleString(key)); // this is only setable internally not a registered setting!! 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 index fdba1c7009bc3..96982f3b68829 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java @@ -94,6 +94,10 @@ public DiskThresholdSettings(Settings settings, ClusterSettings clusterSettings) static final class LowDiskWatermarkValidator implements Setting.Validator { + @Override + public void validate(String value) { + } + @Override public void validate(String value, Map, String> settings) { final String highWatermarkRaw = settings.get(CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING); @@ -113,6 +117,10 @@ public Iterator> settings() { static final class HighDiskWatermarkValidator implements Setting.Validator { + @Override + public void validate(String value) { + } + @Override public void validate(String value, Map, String> settings) { final String lowWatermarkRaw = settings.get(CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING); @@ -132,6 +140,10 @@ public Iterator> settings() { static final class FloodStageValidator implements Setting.Validator { + @Override + public void validate(String value) { + } + @Override public void validate(String value, Map, String> settings) { final String lowWatermarkRaw = settings.get(CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING); diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java index df623aa8a5e07..7a54a7d1e8444 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java @@ -72,14 +72,14 @@ public class FilterAllocationDecider extends AllocationDecider { private static final String CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX = "cluster.routing.allocation.include"; private static final String CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX = "cluster.routing.allocation.exclude"; public static final Setting.AffixSetting CLUSTER_ROUTING_REQUIRE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator( + key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); public static final Setting.AffixSetting CLUSTER_ROUTING_INCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator( + key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); public static final Setting.AffixSettingCLUSTER_ROUTING_EXCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator( + key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); /** * The set of {@link RecoverySource.Type} values for which the diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 8671a6c80631d..dc40257ee7116 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -58,8 +58,6 @@ public abstract class AbstractScopedSettings extends AbstractComponent { private static final Pattern KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])*[-\\w]+$"); private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$"); private static final Pattern AFFIX_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+[*](?:[.][-\\w]+)+$"); - private static final Predicate WILDCARD_SETTING = key -> key.endsWith("*"); - private static final Predicate NOT_WILDCARD_SETTING = key -> !WILDCARD_SETTING.test(key); protected AbstractScopedSettings( final Settings settings, @@ -354,30 +352,6 @@ public synchronized void addSettingsUpdateConsumer(Setting setting, Consu addSettingsUpdateConsumer(setting, consumer, (s) -> {}); } - /** - * Validates that all settings are registered and valid against other valid settings. - * - * @param settings the settings to validates - * @param others the other settings to validate against - * @param validateDependencies true if dependent settings should be validated - * @see Setting#getSettingsDependencies(String) - */ - public final void validate(Settings settings, Settings others, boolean validateDependencies) { - final Settings.Builder validSettings = Settings.builder(); - for (String key : others.keySet()) { - if (NOT_WILDCARD_SETTING.test(key) && settings.keySet().contains(key) == false) { - try { - validate(key, others, validateDependencies); - validSettings.copy(key, others); - } catch (Exception ignored) { - // ignore invalid settings - } - } - } - final Settings toValidate = validSettings.put(settings.filter(NOT_WILDCARD_SETTING)).build(); - validate(toValidate, validateDependencies); - } - /** * Validates that all settings are registered and valid. * @@ -742,7 +716,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin } else if (get(key) == null) { throw new IllegalArgumentException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { - validate(toApply, target.build(), false); + get(key).validateWithoutDependencies(toApply); settingsBuilder.copy(key, toApply); updates.copy(key, toApply); changed = true; 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 23984e58749f7..85bfc2ffff9ec 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -186,7 +186,7 @@ private void checkPropertyRequiresIndexScope(final EnumSet properties, * @param properties properties for this setting like scope, filtering... */ public Setting(Key key, Function defaultValue, Function parser, Property... properties) { - this(key, defaultValue, parser, (v, s) -> {}, properties); + this(key, defaultValue, parser, v -> {}, properties); } /** @@ -246,7 +246,7 @@ public Setting(String key, Function defaultValue, Function fallbackSetting, Function parser, Property... properties) { - this(key, fallbackSetting, fallbackSetting::getRaw, parser, (v, m) -> {}, properties); + this(key, fallbackSetting, fallbackSetting::getRaw, parser, v -> {}, properties); } /** @@ -354,6 +354,15 @@ boolean hasComplexMatcher() { return isGroupSetting(); } + /** + * Validate the setting value without dependencies. + * + * @param settings the settings + */ + void validateWithoutDependencies(Settings settings) { + validator.validate(get(settings, false)); + } + /** * Returns the default value string representation for this setting. * @param settings a settings object for settings that has a default value depending on another setting if available @@ -815,13 +824,22 @@ public Map getAsMap(Settings settings) { @FunctionalInterface public interface Validator { + /** + * The validation routine for this validator. + * + * @param value the value of this setting + * + */ + void validate(T value); + /** * The validation routine for this validator. * * @param value the value of this setting * @param settings a map from the settings specified by {@link #settings()}} to their values */ - void validate(T value, Map, T> settings); + default void validate(T value, Map, T> settings) { + } /** * The settings needed by this validator. @@ -1039,7 +1057,7 @@ public static Setting simpleString( return new Setting<>(key, fallback, parser, properties); } - public static Setting simpleString(String key, Validator validator, Property... properties) { + public static Setting simpleStringWithValidator(String key, Validator validator, Property... properties) { return new Setting<>(new SimpleKey(key), null, s -> "", Function.identity(), validator, properties); } @@ -1283,7 +1301,7 @@ private ListSetting( fallbackSetting, (s) -> Setting.arrayToParsableString(defaultStringValue.apply(s)), parser, - (v,s) -> {}, + v -> {}, properties); this.defaultStringValue = defaultStringValue; } @@ -1341,7 +1359,7 @@ public static Setting timeSetting( fallbackSetting, fallbackSetting::getRaw, minTimeValueParser(key, minValue), - (v, s) -> {}, + v -> {}, properties); } diff --git a/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java b/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java index 45f53006ecd59..1a46652bafebc 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java +++ b/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java @@ -77,6 +77,10 @@ public final class AutoQueueAdjustingExecutorBuilder extends ExecutorBuilder Setting.parseInt(s, 0, minSizeKey), new Setting.Validator() { + @Override + public void validate(Integer value) { + } + @Override public void validate(Integer value, Map, Integer> settings) { if (value > settings.get(tempMaxQueueSizeSetting)) { @@ -96,6 +100,10 @@ public Iterator> settings() { Integer.toString(maxQueueSize), (s) -> Setting.parseInt(s, 0, maxSizeKey), new Setting.Validator() { + @Override + public void validate(Integer value) { + } + @Override public void validate(Integer value, Map, Integer> settings) { if (value < settings.get(tempMinQueueSizeSetting)) { diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 5fcd369a8a433..a2659f9475327 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -29,12 +29,16 @@ import org.elasticsearch.test.ESTestCase; import java.util.ArrayList; +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; import java.util.stream.Stream; +import static java.util.Arrays.asList; import static org.elasticsearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; @@ -183,12 +187,20 @@ public void testUpdateWithUnknownAndSettings() { final int numberOfInvalidSettings = randomIntBetween(0, 7); final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); for (int i = 0; i < numberOfInvalidSettings; i++) { - final Setting invalidSetting = Setting.simpleString( - "invalid.setting" + i, - (value, settings) -> { + final Setting invalidSetting = Setting.simpleStringWithValidator( + "invalid.setting" + i, + new Setting.Validator() { + @Override + public void validate(String value) { throw new IllegalArgumentException("invalid"); - }, - Property.NodeScope); + } + + @Override + public void validate(String value, Map, String> settings) { + throw new IllegalArgumentException("invalid"); + } + }, + Property.NodeScope); invalidSettings.add(invalidSetting); } @@ -355,10 +367,18 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown final int numberOfInvalidSettings = randomIntBetween(0, 7); final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); for (int i = 0; i < numberOfInvalidSettings; i++) { - final Setting invalidSetting = Setting.simpleString( + final Setting invalidSetting = Setting.simpleStringWithValidator( "invalid.setting" + i, - (value, settings) -> { - throw new IllegalArgumentException("invalid"); + new Setting.Validator() { + @Override + public void validate(String value) { + throw new IllegalArgumentException("invalid"); + } + + @Override + public void validate(String value, Map, String> settings) { + throw new IllegalArgumentException("invalid"); + } }, Property.NodeScope); invalidSettings.add(invalidSetting); @@ -471,4 +491,74 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown } } + private static class FooLowSettingValidator implements Setting.Validator { + @Override + public void validate(Integer value) { + } + + @Override + public void validate(Integer low, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_HIGH) && low > settings.get(SETTING_FOO_HIGH)) { + throw new IllegalArgumentException("[low]=" + low + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH)); + } + } + + @Override + public Iterator> settings() { + return asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); + } + } + + private static class FooHighSettingValidator implements Setting.Validator { + @Override + public void validate(Integer value) { + } + + @Override + public void validate(Integer high, Map, Integer> settings) { + if (settings.containsKey(SETTING_FOO_LOW) && high < settings.get(SETTING_FOO_LOW)) { + throw new IllegalArgumentException("[high]=" + high + " is lower than [low]=" + settings.get(SETTING_FOO_LOW)); + } + } + + @Override + public Iterator> settings() { + return asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); + } + } + + private static final Setting SETTING_FOO_LOW = new Setting<>("foo.low", "10", + Integer::valueOf, new FooLowSettingValidator(), Property.Dynamic, Setting.Property.NodeScope); + private static final Setting SETTING_FOO_HIGH = new Setting<>("foo.high", "100", + Integer::valueOf, new FooHighSettingValidator(), Property.Dynamic, Setting.Property.NodeScope); + + public void testUpdateOfValidationDependantSettings() { + final ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(asList(SETTING_FOO_LOW, SETTING_FOO_HIGH))); + final SettingsUpdater updater = new SettingsUpdater(settings); + final MetaData.Builder metaData = MetaData.builder().persistentSettings(Settings.EMPTY).transientSettings(Settings.EMPTY); + + ClusterState cluster = ClusterState.builder(new ClusterName("cluster")).metaData(metaData).build(); + + cluster = updater.updateSettings(cluster, Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), Settings.EMPTY, logger); + assertThat(cluster.getMetaData().settings().get(SETTING_FOO_LOW.getKey()), equalTo("20")); + + cluster = updater.updateSettings(cluster, Settings.builder().put(SETTING_FOO_HIGH.getKey(), 40).build(), Settings.EMPTY, logger); + assertThat(cluster.getMetaData().settings().get(SETTING_FOO_LOW.getKey()), equalTo("20")); + assertThat(cluster.getMetaData().settings().get(SETTING_FOO_HIGH.getKey()), equalTo("40")); + + cluster = updater.updateSettings(cluster, Settings.builder().put(SETTING_FOO_LOW.getKey(), 5).build(), Settings.EMPTY, logger); + assertThat(cluster.getMetaData().settings().get(SETTING_FOO_LOW.getKey()), equalTo("5")); + assertThat(cluster.getMetaData().settings().get(SETTING_FOO_HIGH.getKey()), equalTo("40")); + + cluster = updater.updateSettings(cluster, Settings.builder().put(SETTING_FOO_HIGH.getKey(), 8).build(), Settings.EMPTY, logger); + assertThat(cluster.getMetaData().settings().get(SETTING_FOO_LOW.getKey()), equalTo("5")); + assertThat(cluster.getMetaData().settings().get(SETTING_FOO_HIGH.getKey()), equalTo("8")); + + final ClusterState finalCluster = cluster; + Exception exception = expectThrows(IllegalArgumentException.class, () -> + updater.updateSettings(finalCluster, Settings.builder().put(SETTING_FOO_HIGH.getKey(), 2).build(), Settings.EMPTY, logger)); + + assertThat(exception.getMessage(), equalTo("[high]=2 is lower than [low]=5")); + } + } diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 0387fb13331c4..e02c5d46049b8 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -37,7 +37,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -51,9 +50,7 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; -import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.startsWith; -import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.sameInstance; @@ -513,94 +510,6 @@ public void testApply() { assertEquals(15, bC.get()); } - private static final Setting FOO_BAR_LOW_SETTING = new Setting<>( - "foo.bar.low", - "1", - Integer::parseInt, - new FooBarLowValidator(), - Property.Dynamic, - Property.NodeScope); - - private static final Setting FOO_BAR_HIGH_SETTING = new Setting<>( - "foo.bar.high", - "2", - Integer::parseInt, - new FooBarHighValidator(), - Property.Dynamic, - Property.NodeScope); - - static class FooBarLowValidator implements Setting.Validator { - @Override - public void validate(Integer value, Map, Integer> settings) { - final int high = settings.get(FOO_BAR_HIGH_SETTING); - if (value > high) { - throw new IllegalArgumentException("low [" + value + "] more than high [" + high + "]"); - } - } - - @Override - public Iterator> settings() { - return Collections.singletonList(FOO_BAR_HIGH_SETTING).iterator(); - } - } - - static class FooBarHighValidator implements Setting.Validator { - @Override - public void validate(Integer value, Map, Integer> settings) { - final int low = settings.get(FOO_BAR_LOW_SETTING); - if (value < low) { - throw new IllegalArgumentException("high [" + value + "] less than low [" + low + "]"); - } - } - - @Override - public Iterator> settings() { - return Collections.singletonList(FOO_BAR_LOW_SETTING).iterator(); - } - } - - public void testValidator() { - final AbstractScopedSettings service = - new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(FOO_BAR_LOW_SETTING, FOO_BAR_HIGH_SETTING))); - - final AtomicInteger consumerLow = new AtomicInteger(); - final AtomicInteger consumerHigh = new AtomicInteger(); - - service.addSettingsUpdateConsumer(FOO_BAR_LOW_SETTING, consumerLow::set); - - service.addSettingsUpdateConsumer(FOO_BAR_HIGH_SETTING, consumerHigh::set); - - final Settings newSettings = Settings.builder().put("foo.bar.low", 17).put("foo.bar.high", 13).build(); - { - final IllegalArgumentException e = - expectThrows( - IllegalArgumentException.class, - () -> service.validateUpdate(newSettings)); - assertThat(e, hasToString(containsString("illegal value can't update [foo.bar.low] from [1] to [17]"))); - assertNotNull(e.getCause()); - assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); - final IllegalArgumentException cause = (IllegalArgumentException) e.getCause(); - assertThat(cause, hasToString(containsString("low [17] more than high [13]"))); - assertThat(e.getSuppressed(), arrayWithSize(1)); - assertThat(e.getSuppressed()[0], instanceOf(IllegalArgumentException.class)); - final IllegalArgumentException suppressed = (IllegalArgumentException) e.getSuppressed()[0]; - assertThat(suppressed, hasToString(containsString("illegal value can't update [foo.bar.high] from [2] to [13]"))); - assertNotNull(suppressed.getCause()); - assertThat(suppressed.getCause(), instanceOf(IllegalArgumentException.class)); - final IllegalArgumentException suppressedCause = (IllegalArgumentException) suppressed.getCause(); - assertThat(suppressedCause, hasToString(containsString("high [13] less than low [17]"))); - assertThat(consumerLow.get(), equalTo(0)); - assertThat(consumerHigh.get(), equalTo(0)); - } - - { - final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> service.applySettings(newSettings)); - assertThat(e, hasToString(containsString("illegal value can't update [foo.bar.low] from [1] to [17]"))); - assertThat(consumerLow.get(), equalTo(0)); - assertThat(consumerHigh.get(), equalTo(0)); - } - } - public void testGet() { ClusterSettings settings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); // affix setting - complex matcher @@ -1221,87 +1130,4 @@ public List getListValue(final List value) { equalTo(oldSetting.get(settings).stream().map(s -> "new." + s).collect(Collectors.toList()))); } - private static class FooLowSettingValidator implements Setting.Validator { - @Override - public void validate(Integer low, Map, Integer> settings) { - if (settings.containsKey(SETTING_FOO_HIGH) && low > settings.get(SETTING_FOO_HIGH)) { - throw new IllegalArgumentException("[low]=" + low + " is higher than [high]=" + settings.get(SETTING_FOO_HIGH)); - } - } - - @Override - public Iterator> settings() { - return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); - } - } - - private static class FooHighSettingValidator implements Setting.Validator { - @Override - public void validate(Integer high, Map, Integer> settings) { - if (settings.containsKey(SETTING_FOO_LOW) && high < settings.get(SETTING_FOO_LOW)) { - throw new IllegalArgumentException("[high]=" + high + " is lower than [low]=" + settings.get(SETTING_FOO_LOW)); - } - } - - @Override - public Iterator> settings() { - return Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH).iterator(); - } - } - - private static final Setting SETTING_FOO_LOW = new Setting<>("foo.low", "10", - Integer::valueOf, new FooLowSettingValidator(), Setting.Property.NodeScope); - private static final Setting SETTING_FOO_HIGH = new Setting<>("foo.high", "100", - Integer::valueOf, new FooHighSettingValidator(), Setting.Property.NodeScope); - - public void testUpdateOfValidationDependantSettings() { - ClusterSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(SETTING_FOO_LOW, SETTING_FOO_HIGH))); - - Settings.Builder target = Settings.builder(); - - { - final Settings.Builder updates = Settings.builder(); - assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), target, updates, "transient")); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - } - - { - final Settings.Builder updates = Settings.builder(); - assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), target, updates, "transient")); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - } - - { - final Settings.Builder updates = Settings.builder(); - assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_HIGH.getKey(), 40).build(), target, updates, "transient")); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); - assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); - assertNull(updates.get(SETTING_FOO_LOW.getKey())); - assertThat(updates.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); - } - - { - final Settings.Builder updates = Settings.builder(); - assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_LOW.getKey(), 5).build(), target, updates, "transient")); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("5")); - assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("40")); - assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("5")); - assertNull(updates.get(SETTING_FOO_HIGH.getKey())); - } - - { - final Settings.Builder updates = Settings.builder(); - assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_HIGH.getKey(), 8).build(), target, updates, "transient")); - assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("5")); - assertThat(target.get(SETTING_FOO_HIGH.getKey()), equalTo("8")); - assertNull(updates.get(SETTING_FOO_LOW.getKey())); - assertThat(updates.get(SETTING_FOO_HIGH.getKey()), equalTo("8")); - } - - Exception exception = expectThrows(IllegalArgumentException.class, () -> - service.updateSettings(Settings.builder().put(SETTING_FOO_HIGH.getKey(), 2).build(), target, Settings.builder(), "transient")); - assertThat(exception.getMessage(), equalTo("[high]=2 is lower than [low]=5")); - } } diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 99fde0855f94a..a789bc3d2db26 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -203,6 +203,10 @@ static class FooBarValidator implements Setting.Validator { public static boolean invoked; + @Override + public void validate(String value) { + } + @Override public void validate(String value, Map, String> settings) { invoked = true; diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java index 997f04e33bd77..7f36b91bcc42a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java @@ -132,7 +132,7 @@ private XPackSettings() { * Do not allow insecure hashing algorithms to be used for password hashing */ public static final Setting PASSWORD_HASHING_ALGORITHM = new Setting<>( - "xpack.security.authc.password_hashing.algorithm", "bcrypt", Function.identity(), (v, s) -> { + "xpack.security.authc.password_hashing.algorithm", "bcrypt", Function.identity(), v -> { if (Hasher.getAvailableAlgoStoredHash().contains(v.toLowerCase(Locale.ROOT)) == false) { throw new IllegalArgumentException("Invalid algorithm: " + v + ". Valid values for password hashing are " + Hasher.getAvailableAlgoStoredHash().toString()); diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java index c33be6dea8510..a5cfe40cf39cc 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java @@ -28,7 +28,7 @@ public abstract class Exporter implements AutoCloseable { private static final Setting.AffixSetting TYPE_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","type", - (key) -> Setting.simpleString(key, (v, s) -> { + key -> Setting.simpleStringWithValidator(key, v -> { switch (v) { case "": case "http": diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java index f4f97df1d4fd8..13fb749f2f468 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java @@ -34,7 +34,7 @@ public class HttpSettings { private static final String SSL_KEY_PREFIX = "xpack.http.ssl."; static final Setting PROXY_HOST = Setting.simpleString(PROXY_HOST_KEY, Property.NodeScope); - static final Setting PROXY_SCHEME = Setting.simpleString(PROXY_SCHEME_KEY, (v, s) -> Scheme.parse(v), Property.NodeScope); + static final Setting PROXY_SCHEME = Setting.simpleStringWithValidator(PROXY_SCHEME_KEY, Scheme::parse, Property.NodeScope); static final Setting PROXY_PORT = Setting.intSetting(PROXY_PORT_KEY, 0, 0, 0xFFFF, Property.NodeScope); static final Setting MAX_HTTP_RESPONSE_SIZE = Setting.byteSizeSetting("xpack.http.max_response_size", From 75048ac102339437128bb8ddbcf6b09c24041103 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 8 Oct 2018 23:18:18 +0200 Subject: [PATCH 14/28] Reduce diffs with "master" branch (#28309) --- .../common/settings/AbstractScopedSettings.java | 2 +- .../java/org/elasticsearch/common/settings/Setting.java | 8 ++++---- .../java/org/elasticsearch/xpack/core/XPackSettings.java | 2 +- .../elasticsearch/xpack/monitoring/exporter/Exporter.java | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index dc40257ee7116..6c820623013da 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -716,7 +716,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin } else if (get(key) == null) { throw new IllegalArgumentException(type + " setting [" + key + "], not recognized"); } else if (isDelete == false && canUpdate.test(key)) { - get(key).validateWithoutDependencies(toApply); + get(key).validateWithoutDependencies(toApply); // we might not have a full picture here do to a dependency validation settingsBuilder.copy(key, toApply); updates.copy(key, toApply); changed = true; 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 85bfc2ffff9ec..6b2321932d79c 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -186,7 +186,7 @@ private void checkPropertyRequiresIndexScope(final EnumSet properties, * @param properties properties for this setting like scope, filtering... */ public Setting(Key key, Function defaultValue, Function parser, Property... properties) { - this(key, defaultValue, parser, v -> {}, properties); + this(key, defaultValue, parser, (v) -> {}, properties); } /** @@ -246,7 +246,7 @@ public Setting(String key, Function defaultValue, Function fallbackSetting, Function parser, Property... properties) { - this(key, fallbackSetting, fallbackSetting::getRaw, parser, v -> {}, properties); + this(key, fallbackSetting, fallbackSetting::getRaw, parser, (v) -> {}, properties); } /** @@ -1301,7 +1301,7 @@ private ListSetting( fallbackSetting, (s) -> Setting.arrayToParsableString(defaultStringValue.apply(s)), parser, - v -> {}, + (v) -> {}, properties); this.defaultStringValue = defaultStringValue; } @@ -1359,7 +1359,7 @@ public static Setting timeSetting( fallbackSetting, fallbackSetting::getRaw, minTimeValueParser(key, minValue), - v -> {}, + (v) -> {}, properties); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java index 7f36b91bcc42a..7d64ece3bdb12 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java @@ -132,7 +132,7 @@ private XPackSettings() { * Do not allow insecure hashing algorithms to be used for password hashing */ public static final Setting PASSWORD_HASHING_ALGORITHM = new Setting<>( - "xpack.security.authc.password_hashing.algorithm", "bcrypt", Function.identity(), v -> { + "xpack.security.authc.password_hashing.algorithm", "bcrypt", Function.identity(), (v) -> { if (Hasher.getAvailableAlgoStoredHash().contains(v.toLowerCase(Locale.ROOT)) == false) { throw new IllegalArgumentException("Invalid algorithm: " + v + ". Valid values for password hashing are " + Hasher.getAvailableAlgoStoredHash().toString()); diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java index a5cfe40cf39cc..41bf160e20986 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java @@ -28,7 +28,7 @@ public abstract class Exporter implements AutoCloseable { private static final Setting.AffixSetting TYPE_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","type", - key -> Setting.simpleStringWithValidator(key, v -> { + (key) -> Setting.simpleStringWithValidator(key, (v) -> { switch (v) { case "": case "http": From 158c482e777e04cbff4b5ab568d7e98620da7606 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Tue, 9 Oct 2018 09:13:26 +0200 Subject: [PATCH 15/28] Improve Java documentation (#28309) --- .../java/org/elasticsearch/common/settings/Setting.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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 6b2321932d79c..07426c127b78f 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -355,9 +355,8 @@ boolean hasComplexMatcher() { } /** - * Validate the setting value without dependencies. - * - * @param settings the settings + * Validate the current setting value only without dependencies with {@link Setting.Validator#validate(Object)}. + * @param settings a settings object for settings that has a default value depending on another setting if available */ void validateWithoutDependencies(Settings settings) { validator.validate(get(settings, false)); @@ -828,7 +827,6 @@ public interface Validator { * The validation routine for this validator. * * @param value the value of this setting - * */ void validate(T value); From c195f74cc837c0fd293b1d184cb47e12ec8fa7f3 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Tue, 9 Oct 2018 09:23:48 +0200 Subject: [PATCH 16/28] Extract methods to create unknown and invalid settings in tests (#28309) --- .../settings/SettingsUpdaterTests.java | 84 +++++++++---------- 1 file changed, 38 insertions(+), 46 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index a2659f9475327..561fa0674a623 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -185,32 +185,11 @@ public void testUpdateWithUnknownAndSettings() { // these are invalid settings that exist as either persistent or transient settings final int numberOfInvalidSettings = randomIntBetween(0, 7); - final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); - for (int i = 0; i < numberOfInvalidSettings; i++) { - final Setting invalidSetting = Setting.simpleStringWithValidator( - "invalid.setting" + i, - new Setting.Validator() { - @Override - public void validate(String value) { - throw new IllegalArgumentException("invalid"); - } - - @Override - public void validate(String value, Map, String> settings) { - throw new IllegalArgumentException("invalid"); - } - }, - Property.NodeScope); - invalidSettings.add(invalidSetting); - } + final List> invalidSettings = createInvalidSettings(numberOfInvalidSettings); // these are unknown settings that exist as either persistent or transient settings final int numberOfUnknownSettings = randomIntBetween(0, 7); - final List> unknownSettings = new ArrayList<>(numberOfUnknownSettings); - for (int i = 0; i < numberOfUnknownSettings; i++) { - final Setting unknownSetting = Setting.simpleString("unknown.setting" + i, Property.NodeScope); - unknownSettings.add(unknownSetting); - } + final List> unknownSettings = createUnknownSettings(numberOfUnknownSettings); final Settings.Builder existingPersistentSettings = Settings.builder(); final Settings.Builder existingTransientSettings = Settings.builder(); @@ -365,32 +344,11 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown // these are invalid settings that exist as either persistent or transient settings final int numberOfInvalidSettings = randomIntBetween(0, 7); - final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); - for (int i = 0; i < numberOfInvalidSettings; i++) { - final Setting invalidSetting = Setting.simpleStringWithValidator( - "invalid.setting" + i, - new Setting.Validator() { - @Override - public void validate(String value) { - throw new IllegalArgumentException("invalid"); - } - - @Override - public void validate(String value, Map, String> settings) { - throw new IllegalArgumentException("invalid"); - } - }, - Property.NodeScope); - invalidSettings.add(invalidSetting); - } + final List> invalidSettings = createInvalidSettings(numberOfInvalidSettings); // these are unknown settings that exist as either persistent or transient settings final int numberOfUnknownSettings = randomIntBetween(0, 7); - final List> unknownSettings = new ArrayList<>(numberOfUnknownSettings); - for (int i = 0; i < numberOfUnknownSettings; i++) { - final Setting unknownSetting = Setting.simpleString("unknown.setting" + i, Property.NodeScope); - unknownSettings.add(unknownSetting); - } + final List> unknownSettings = createUnknownSettings(numberOfUnknownSettings); final Settings.Builder existingPersistentSettings = Settings.builder(); final Settings.Builder existingTransientSettings = Settings.builder(); @@ -491,6 +449,40 @@ public void validate(String value, Map, String> settings) { } } + private static List> createUnknownSettings(int numberOfUnknownSettings) { + final List> unknownSettings = new ArrayList<>(numberOfUnknownSettings); + for (int i = 0; i < numberOfUnknownSettings; i++) { + final Setting unknownSetting = Setting.simpleString("unknown.setting" + i, Property.NodeScope); + unknownSettings.add(unknownSetting); + } + return unknownSettings; + } + + private static List> createInvalidSettings(int numberOfInvalidSettings) { + final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); + for (int i = 0; i < numberOfInvalidSettings; i++) { + final Setting invalidSetting = createInvalidSetting(i); + invalidSettings.add(invalidSetting); + } + return invalidSettings; + } + + private static Setting createInvalidSetting(int index) { + return Setting.simpleStringWithValidator("invalid.setting" + index, + new Setting.Validator() { + @Override + public void validate(String value) { + throw new IllegalArgumentException("invalid"); + } + + @Override + public void validate(String value, Map, String> settings) { + throw new IllegalArgumentException("invalid"); + } + }, + Property.NodeScope); + } + private static class FooLowSettingValidator implements Setting.Validator { @Override public void validate(Integer value) { From 8d2e144619221ff8d2cddd06c0813dea88c25f65 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Fri, 26 Oct 2018 09:44:41 +0200 Subject: [PATCH 17/28] Fix compilation after rebasing with master (#28309) --- .../java/org/elasticsearch/common/settings/SettingTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 35a1d96e02eb4..f9e788c19a80d 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -909,7 +909,7 @@ public void testAffixMapUpdateWithNullSettingValue() { final Setting.AffixSetting affixSetting = Setting.prefixKeySetting("prefix" + ".", - (key) -> Setting.simpleString(key, (value, map) -> {}, Property.Dynamic, Property.NodeScope)); + (key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope)); final Consumer> consumer = (map) -> {}; final BiConsumer validator = (s1, s2) -> {}; From 88f42edb12f8312309d04cbdcadb943186447b30 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Fri, 26 Oct 2018 10:50:40 +0200 Subject: [PATCH 18/28] Add missing validation step without dependencies validation (#28309) --- .../src/main/java/org/elasticsearch/common/settings/Setting.java | 1 + 1 file changed, 1 insertion(+) 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 517ce4c164f26..d713c91e3fcaa 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -422,6 +422,7 @@ private T get(Settings settings, boolean validate) { } else { map = Collections.emptyMap(); } + validator.validate(parsed); validator.validate(parsed, map); } return parsed; From 5bded6e3ca039d678bf96ecb535a0f58c88d47fe Mon Sep 17 00:00:00 2001 From: David Turner Date: Tue, 30 Oct 2018 16:35:57 +0100 Subject: [PATCH 19/28] Improve JavaDoc comment (#28309). Co-Authored-By: cbismuth --- .../main/java/org/elasticsearch/common/settings/Setting.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d713c91e3fcaa..f41b28f657b0b 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -823,7 +823,7 @@ public Map getAsMap(Settings settings) { public interface Validator { /** - * The validation routine for this validator. + * Validate this setting's value in isolation. * * @param value the value of this setting */ From 9890ab7b5a3033a61948c14c515268662a554c23 Mon Sep 17 00:00:00 2001 From: David Turner Date: Tue, 30 Oct 2018 16:36:49 +0100 Subject: [PATCH 20/28] Improve JavaDoc comment (#28309) Co-Authored-By: cbismuth --- .../main/java/org/elasticsearch/common/settings/Setting.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f41b28f657b0b..879fdb025f6a7 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -830,7 +830,7 @@ public interface Validator { void validate(T value); /** - * The validation routine for this validator. + * Validate this setting against its dependencies, specified by {@link #settings()}. * * @param value the value of this setting * @param settings a map from the settings specified by {@link #settings()}} to their values From f277ae9a5345847e881278d5d8de13e81a31772e Mon Sep 17 00:00:00 2001 From: David Turner Date: Tue, 30 Oct 2018 19:01:33 +0100 Subject: [PATCH 21/28] Fix typo in test name (#28309) Co-Authored-By: cbismuth --- .../action/admin/cluster/settings/SettingsUpdaterTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 561fa0674a623..ba65a71178113 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -524,7 +524,7 @@ public Iterator> settings() { private static final Setting SETTING_FOO_HIGH = new Setting<>("foo.high", "100", Integer::valueOf, new FooHighSettingValidator(), Property.Dynamic, Setting.Property.NodeScope); - public void testUpdateOfValidationDependantSettings() { + public void testUpdateOfValidationDependentSettings() { final ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(asList(SETTING_FOO_LOW, SETTING_FOO_HIGH))); final SettingsUpdater updater = new SettingsUpdater(settings); final MetaData.Builder metaData = MetaData.builder().persistentSettings(Settings.EMPTY).transientSettings(Settings.EMPTY); From 2efdf1e79f66e43f5b9a1450542b85f018d6a6ae Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Wed, 31 Oct 2018 12:16:36 +0100 Subject: [PATCH 22/28] Split invalid in-isolation/with-dependencies test settings (#28309) --- .../settings/SettingsUpdaterTests.java | 38 ++++++++++++------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 561fa0674a623..bcf9bc3c11282 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -185,11 +185,11 @@ public void testUpdateWithUnknownAndSettings() { // these are invalid settings that exist as either persistent or transient settings final int numberOfInvalidSettings = randomIntBetween(0, 7); - final List> invalidSettings = createInvalidSettings(numberOfInvalidSettings); + final List> invalidSettings = invalidSettings(numberOfInvalidSettings); // these are unknown settings that exist as either persistent or transient settings final int numberOfUnknownSettings = randomIntBetween(0, 7); - final List> unknownSettings = createUnknownSettings(numberOfUnknownSettings); + final List> unknownSettings = unknownSettings(numberOfUnknownSettings); final Settings.Builder existingPersistentSettings = Settings.builder(); final Settings.Builder existingTransientSettings = Settings.builder(); @@ -344,11 +344,11 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown // these are invalid settings that exist as either persistent or transient settings final int numberOfInvalidSettings = randomIntBetween(0, 7); - final List> invalidSettings = createInvalidSettings(numberOfInvalidSettings); + final List> invalidSettings = invalidSettings(numberOfInvalidSettings); // these are unknown settings that exist as either persistent or transient settings final int numberOfUnknownSettings = randomIntBetween(0, 7); - final List> unknownSettings = createUnknownSettings(numberOfUnknownSettings); + final List> unknownSettings = unknownSettings(numberOfUnknownSettings); final Settings.Builder existingPersistentSettings = Settings.builder(); final Settings.Builder existingTransientSettings = Settings.builder(); @@ -449,35 +449,47 @@ public void testRemovingArchivedSettingsDoesNotRemoveNonArchivedInvalidOrUnknown } } - private static List> createUnknownSettings(int numberOfUnknownSettings) { + private static List> unknownSettings(int numberOfUnknownSettings) { final List> unknownSettings = new ArrayList<>(numberOfUnknownSettings); for (int i = 0; i < numberOfUnknownSettings; i++) { - final Setting unknownSetting = Setting.simpleString("unknown.setting" + i, Property.NodeScope); - unknownSettings.add(unknownSetting); + unknownSettings.add(Setting.simpleString("unknown.setting" + i, Property.NodeScope)); } return unknownSettings; } - private static List> createInvalidSettings(int numberOfInvalidSettings) { + private static List> invalidSettings(int numberOfInvalidSettings) { final List> invalidSettings = new ArrayList<>(numberOfInvalidSettings); for (int i = 0; i < numberOfInvalidSettings; i++) { - final Setting invalidSetting = createInvalidSetting(i); - invalidSettings.add(invalidSetting); + invalidSettings.add(randomBoolean() ? invalidInIsolationSetting(i) : invalidWithDependenciesSetting(i)); } return invalidSettings; } - private static Setting createInvalidSetting(int index) { + private static Setting invalidInIsolationSetting(int index) { return Setting.simpleStringWithValidator("invalid.setting" + index, new Setting.Validator() { @Override public void validate(String value) { - throw new IllegalArgumentException("invalid"); + throw new IllegalArgumentException("Invalid in isolation setting"); } @Override public void validate(String value, Map, String> settings) { - throw new IllegalArgumentException("invalid"); + } + }, + Property.NodeScope); + } + + private static Setting invalidWithDependenciesSetting(int index) { + return Setting.simpleStringWithValidator("invalid.setting" + index, + new Setting.Validator() { + @Override + public void validate(String value) { + } + + @Override + public void validate(String value, Map, String> settings) { + throw new IllegalArgumentException("Invalid with dependencies setting"); } }, Property.NodeScope); From 3e584737f71f6923791243df8c3700eb4a502b48 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Wed, 31 Oct 2018 12:33:10 +0100 Subject: [PATCH 23/28] Split invalid in-isolation/with-dependencies setting validation (#28309) --- .../elasticsearch/common/settings/SettingTests.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index f9e788c19a80d..22d19e6b78161 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -204,16 +204,18 @@ public void testValidateStringSetting() { static class FooBarValidator implements Setting.Validator { - public static boolean invoked; + public static boolean invokedInIsolation; + public static boolean invokedWithDependencies; @Override public void validate(String value) { + invokedInIsolation = true; + assertThat(value, equalTo("foo.bar value")); } @Override public void validate(String value, Map, String> settings) { - invoked = true; - assertThat(value, equalTo("foo.bar value")); + invokedWithDependencies = true; assertTrue(settings.keySet().contains(BAZ_QUX_SETTING)); assertThat(settings.get(BAZ_QUX_SETTING), equalTo("baz.qux value")); assertTrue(settings.keySet().contains(QUUX_QUUZ_SETTING)); @@ -234,7 +236,8 @@ public void testValidator() { .put("quux.quuz", "quux.quuz value") .build(); FOO_BAR_SETTING.get(settings); - assertTrue(FooBarValidator.invoked); + assertTrue(FooBarValidator.invokedInIsolation); + assertTrue(FooBarValidator.invokedWithDependencies); } public void testUpdateNotDynamic() { From 43840f138129d15ec4322f03db33379631cb3a4b Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Wed, 31 Oct 2018 12:49:11 +0100 Subject: [PATCH 24/28] Fix Checkstyle line length violation (#28309) --- .../cluster/metadata/IndexMetaData.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 0ad8e265d827b..f1dc6b4d50ac3 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java @@ -226,14 +226,14 @@ public Iterator> settings() { public static final String INDEX_ROUTING_INCLUDE_GROUP_PREFIX = "index.routing.allocation.include"; public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude"; public static final Setting.AffixSetting INDEX_ROUTING_REQUIRE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> - Setting.simpleStringWithValidator(key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator(key, (String value) -> + IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleStringWithValidator(key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator(key, (String value) -> + IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_EXCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleStringWithValidator(key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator(key, (String value) -> + IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INITIAL_RECOVERY_GROUP_SETTING = Setting.prefixKeySetting("index.routing.allocation.initial_recovery.", key -> Setting.simpleString(key)); // this is only setable internally not a registered setting!! From 14475777959ed7921aaee0336cedcd37a78b4a9d Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Wed, 31 Oct 2018 18:37:05 +0100 Subject: [PATCH 25/28] Rework simple string setting with validator API (#28309) --- .../customsettings/ExampleCustomSettingsConfig.java | 2 +- .../cluster/metadata/IndexMetaData.java | 12 ++++++------ .../allocation/decider/FilterAllocationDecider.java | 12 ++++++------ .../org/elasticsearch/common/settings/Setting.java | 8 ++------ .../elasticsearch/transport/RemoteClusterAware.java | 1 - .../admin/cluster/settings/SettingsUpdaterTests.java | 4 ++-- .../xpack/monitoring/exporter/Exporter.java | 2 +- .../xpack/watcher/common/http/HttpSettings.java | 2 +- 8 files changed, 19 insertions(+), 24 deletions(-) diff --git a/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java b/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java index 9beb709e2fb84..ff9fc2c7a5e13 100644 --- a/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java +++ b/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java @@ -49,7 +49,7 @@ public class ExampleCustomSettingsConfig { /** * A string setting that can be dynamically updated and that is validated by some logic */ - static final Setting VALIDATED_SETTING = Setting.simpleStringWithValidator("custom.validated", (value) -> { + static final Setting VALIDATED_SETTING = Setting.simpleString("custom.validated", (value) -> { if (value != null && value.contains("forbidden")) { throw new IllegalArgumentException("Setting must not contain [forbidden]"); } 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 f1dc6b4d50ac3..82c3e8a91fe53 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java @@ -226,14 +226,14 @@ public Iterator> settings() { public static final String INDEX_ROUTING_INCLUDE_GROUP_PREFIX = "index.routing.allocation.include"; public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude"; public static final Setting.AffixSetting INDEX_ROUTING_REQUIRE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator(key, (String value) -> - IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> + Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator(key, (String value) -> - IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> + Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_EXCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator(key, (String value) -> - IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> + Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INITIAL_RECOVERY_GROUP_SETTING = Setting.prefixKeySetting("index.routing.allocation.initial_recovery.", key -> Setting.simpleString(key)); // this is only setable internally not a registered setting!! diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java index 96de1c0534f12..961a06ad94843 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java @@ -72,14 +72,14 @@ public class FilterAllocationDecider extends AllocationDecider { private static final String CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX = "cluster.routing.allocation.include"; private static final String CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX = "cluster.routing.allocation.exclude"; public static final Setting.AffixSetting CLUSTER_ROUTING_REQUIRE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator( - key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> + Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); public static final Setting.AffixSetting CLUSTER_ROUTING_INCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator( - key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> + Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); public static final Setting.AffixSettingCLUSTER_ROUTING_EXCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> Setting.simpleStringWithValidator( - key, (String value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> + Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); /** * The set of {@link RecoverySource.Type} values for which the 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 879fdb025f6a7..4f049d7d1ab23 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -1038,8 +1038,8 @@ public static Setting simpleString(String key, Property... properties) { return new Setting<>(key, s -> "", Function.identity(), properties); } - public static Setting simpleString(String key, Function parser, Property... properties) { - return new Setting<>(key, s -> "", parser, properties); + public static Setting simpleString(String key, Validator validator, Property... properties) { + return new Setting<>(new SimpleKey(key), null, s -> "", Function.identity(), validator, properties); } public static Setting simpleString(String key, Setting fallback, Property... properties) { @@ -1054,10 +1054,6 @@ public static Setting simpleString( return new Setting<>(key, fallback, parser, properties); } - public static Setting simpleStringWithValidator(String key, Validator validator, Property... properties) { - return new Setting<>(new SimpleKey(key), null, s -> "", Function.identity(), validator, properties); - } - /** * Creates a new Setting instance with a String value * diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java index f643a91fb4856..e549b33baa415 100644 --- a/server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java +++ b/server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java @@ -122,7 +122,6 @@ public String getKey(final String key) { if (Strings.hasLength(s)) { parsePort(s); } - return s; }, Setting.Property.Deprecated, Setting.Property.Dynamic, diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java index 214a6a58c8175..b8a4d993caa7c 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java @@ -466,7 +466,7 @@ private static List> invalidSettings(int numberOfInvalidSettings } private static Setting invalidInIsolationSetting(int index) { - return Setting.simpleStringWithValidator("invalid.setting" + index, + return Setting.simpleString("invalid.setting" + index, new Setting.Validator() { @Override public void validate(String value) { @@ -481,7 +481,7 @@ public void validate(String value, Map, String> settings) { } private static Setting invalidWithDependenciesSetting(int index) { - return Setting.simpleStringWithValidator("invalid.setting" + index, + return Setting.simpleString("invalid.setting" + index, new Setting.Validator() { @Override public void validate(String value) { diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java index 41bf160e20986..8b000374987ae 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java @@ -28,7 +28,7 @@ public abstract class Exporter implements AutoCloseable { private static final Setting.AffixSetting TYPE_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","type", - (key) -> Setting.simpleStringWithValidator(key, (v) -> { + (key) -> Setting.simpleString(key, (v) -> { switch (v) { case "": case "http": diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java index 13fb749f2f468..af4a20d596cd0 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpSettings.java @@ -34,7 +34,7 @@ public class HttpSettings { private static final String SSL_KEY_PREFIX = "xpack.http.ssl."; static final Setting PROXY_HOST = Setting.simpleString(PROXY_HOST_KEY, Property.NodeScope); - static final Setting PROXY_SCHEME = Setting.simpleStringWithValidator(PROXY_SCHEME_KEY, Scheme::parse, Property.NodeScope); + static final Setting PROXY_SCHEME = Setting.simpleString(PROXY_SCHEME_KEY, Scheme::parse, Property.NodeScope); static final Setting PROXY_PORT = Setting.intSetting(PROXY_PORT_KEY, 0, 0, 0xFFFF, Property.NodeScope); static final Setting MAX_HTTP_RESPONSE_SIZE = Setting.byteSizeSetting("xpack.http.max_response_size", From bfcd8283cb4cb58fa48cec68e2576fc02d57d818 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 5 Nov 2018 13:03:38 +0100 Subject: [PATCH 26/28] Remove unnecessary parentheses around lambdas (#28309) --- .../customsettings/ExampleCustomSettingsConfig.java | 2 +- .../cluster/metadata/IndexMetaData.java | 12 ++++++------ .../allocation/decider/FilterAllocationDecider.java | 12 ++++++------ .../org/elasticsearch/common/settings/Setting.java | 10 +++++----- .../AutoQueueAdjustingExecutorBuilder.java | 4 ++-- .../elasticsearch/common/settings/SettingTests.java | 2 +- .../org/elasticsearch/xpack/core/XPackSettings.java | 2 +- .../xpack/monitoring/exporter/Exporter.java | 12 ++++++------ 8 files changed, 28 insertions(+), 28 deletions(-) diff --git a/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java b/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java index ff9fc2c7a5e13..ffc7b4366b587 100644 --- a/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java +++ b/plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java @@ -49,7 +49,7 @@ public class ExampleCustomSettingsConfig { /** * A string setting that can be dynamically updated and that is validated by some logic */ - static final Setting VALIDATED_SETTING = Setting.simpleString("custom.validated", (value) -> { + static final Setting VALIDATED_SETTING = Setting.simpleString("custom.validated", value -> { if (value != null && value.contains("forbidden")) { throw new IllegalArgumentException("Setting must not contain [forbidden]"); } 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 82c3e8a91fe53..fc2127354323b 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java @@ -226,14 +226,14 @@ public Iterator> settings() { public static final String INDEX_ROUTING_INCLUDE_GROUP_PREFIX = "index.routing.allocation.include"; public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude"; public static final Setting.AffixSetting INDEX_ROUTING_REQUIRE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", key -> + Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", key -> + Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_EXCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); + Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", key -> + Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope)); public static final Setting.AffixSetting INDEX_ROUTING_INITIAL_RECOVERY_GROUP_SETTING = Setting.prefixKeySetting("index.routing.allocation.initial_recovery.", key -> Setting.simpleString(key)); // this is only setable internally not a registered setting!! diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java index 4e6203eb714e4..7d24d46318585 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java @@ -72,14 +72,14 @@ public class FilterAllocationDecider extends AllocationDecider { private static final String CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX = "cluster.routing.allocation.include"; private static final String CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX = "cluster.routing.allocation.exclude"; public static final Setting.AffixSetting CLUSTER_ROUTING_REQUIRE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", key -> + Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); public static final Setting.AffixSetting CLUSTER_ROUTING_INCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", key -> + Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); public static final Setting.AffixSettingCLUSTER_ROUTING_EXCLUDE_GROUP_SETTING = - Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) -> - Setting.simpleString(key, (value) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); + Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", key -> + Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope)); /** * The set of {@link RecoverySource.Type} values for which the 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 4f049d7d1ab23..c5ca14a41a1a5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -186,7 +186,7 @@ private void checkPropertyRequiresIndexScope(final EnumSet properties, * @param properties properties for this setting like scope, filtering... */ public Setting(Key key, Function defaultValue, Function parser, Property... properties) { - this(key, defaultValue, parser, (v) -> {}, properties); + this(key, defaultValue, parser, v -> {}, properties); } /** @@ -246,7 +246,7 @@ public Setting(String key, Function defaultValue, Function fallbackSetting, Function parser, Property... properties) { - this(key, fallbackSetting, fallbackSetting::getRaw, parser, (v) -> {}, properties); + this(key, fallbackSetting, fallbackSetting::getRaw, parser, v -> {}, properties); } /** @@ -1292,9 +1292,9 @@ private ListSetting( super( new ListKey(key), fallbackSetting, - (s) -> Setting.arrayToParsableString(defaultStringValue.apply(s)), + s -> Setting.arrayToParsableString(defaultStringValue.apply(s)), parser, - (v) -> {}, + v -> {}, properties); this.defaultStringValue = defaultStringValue; } @@ -1352,7 +1352,7 @@ public static Setting timeSetting( fallbackSetting, fallbackSetting::getRaw, minTimeValueParser(key, minValue), - (v) -> {}, + v -> {}, properties); } diff --git a/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java b/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java index 1a46652bafebc..bc17d52bcc236 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java +++ b/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java @@ -75,7 +75,7 @@ public final class AutoQueueAdjustingExecutorBuilder extends ExecutorBuilder( minSizeKey, Integer.toString(minQueueSize), - (s) -> Setting.parseInt(s, 0, minSizeKey), + s -> Setting.parseInt(s, 0, minSizeKey), new Setting.Validator() { @Override public void validate(Integer value) { @@ -98,7 +98,7 @@ public Iterator> settings() { this.maxQueueSizeSetting = new Setting<>( maxSizeKey, Integer.toString(maxQueueSize), - (s) -> Setting.parseInt(s, 0, maxSizeKey), + s -> Setting.parseInt(s, 0, maxSizeKey), new Setting.Validator() { @Override public void validate(Integer value) { diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 22d19e6b78161..38a2b8e98982d 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -912,7 +912,7 @@ public void testAffixMapUpdateWithNullSettingValue() { final Setting.AffixSetting affixSetting = Setting.prefixKeySetting("prefix" + ".", - (key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope)); + key -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope)); final Consumer> consumer = (map) -> {}; final BiConsumer validator = (s1, s2) -> {}; diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java index b90f533c8e6e1..13cc4c121daf6 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackSettings.java @@ -139,7 +139,7 @@ private XPackSettings() { * Do not allow insecure hashing algorithms to be used for password hashing */ public static final Setting PASSWORD_HASHING_ALGORITHM = new Setting<>( - "xpack.security.authc.password_hashing.algorithm", "bcrypt", Function.identity(), (v) -> { + "xpack.security.authc.password_hashing.algorithm", "bcrypt", Function.identity(), v -> { if (Hasher.getAvailableAlgoStoredHash().contains(v.toLowerCase(Locale.ROOT)) == false) { throw new IllegalArgumentException("Invalid algorithm: " + v + ". Valid values for password hashing are " + Hasher.getAvailableAlgoStoredHash().toString()); diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java index 8b000374987ae..9d9619896253f 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java @@ -24,11 +24,11 @@ public abstract class Exporter implements AutoCloseable { private static final Setting.AffixSetting ENABLED_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","enabled", - (key) -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); + key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); private static final Setting.AffixSetting TYPE_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","type", - (key) -> Setting.simpleString(key, (v) -> { + key -> Setting.simpleString(key, v -> { switch (v) { case "": case "http": @@ -46,13 +46,13 @@ public abstract class Exporter implements AutoCloseable { */ public static final Setting.AffixSetting USE_INGEST_PIPELINE_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","use_ingest", - (key) -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); + key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); /** * Every {@code Exporter} allows users to explicitly disable cluster alerts. */ public static final Setting.AffixSetting CLUSTER_ALERTS_MANAGEMENT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.", "cluster_alerts.management.enabled", - (key) -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); + key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope)); /** * Every {@code Exporter} allows users to explicitly disable specific cluster alerts. *

@@ -60,14 +60,14 @@ public abstract class Exporter implements AutoCloseable { */ public static final Setting.AffixSetting> CLUSTER_ALERTS_BLACKLIST_SETTING = Setting .affixKeySetting("xpack.monitoring.exporters.", "cluster_alerts.management.blacklist", - (key) -> Setting.listSetting(key, Collections.emptyList(), Function.identity(), Property.Dynamic, Property.NodeScope)); + key -> Setting.listSetting(key, Collections.emptyList(), Function.identity(), Property.Dynamic, Property.NodeScope)); /** * Every {@code Exporter} allows users to use a different index time format. */ private static final Setting.AffixSetting INDEX_NAME_TIME_FORMAT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","index.name.time_format", - (key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope)); + key -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope)); private static final String INDEX_FORMAT = "YYYY.MM.dd"; From 7c33bfe6eb9cb43412bbae0a723657fd96e0a7b9 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Fri, 16 Nov 2018 12:00:25 +0100 Subject: [PATCH 27/28] Fix Java documentation of the Validator class (#elasticsearch-28309) --- .../org/elasticsearch/common/settings/Setting.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 c5ca14a41a1a5..e7d67319d4910 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -814,8 +814,10 @@ public Map getAsMap(Settings settings) { } /** - * Represents a validator for a setting. 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 {@link Settings} instance. + * 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 + * {@link Settings} instance. * * @param the type of the {@link Setting} */ @@ -830,7 +832,7 @@ public interface Validator { void validate(T value); /** - * Validate this setting against its dependencies, specified by {@link #settings()}. + * Validate this setting against its dependencies, specified by {@link #settings()}. Default implementation is a no operation. * * @param value the value of this setting * @param settings a map from the settings specified by {@link #settings()}} to their values @@ -839,7 +841,7 @@ default void validate(T value, Map, T> settings) { } /** - * The settings needed by this validator. + * The settings needed by this validator. Default value is an empty iterator. * * @return the settings needed to validate; these can be used for cross-settings validation */ From 02a62fba907ae0b2116f42a8a57d739d2a16e032 Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 7 Jan 2019 11:20:05 +0000 Subject: [PATCH 28/28] Minor Javadoc rewording --- .../java/org/elasticsearch/common/settings/Setting.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 21101141ce054..9c3762f857e4a 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -832,7 +832,8 @@ public interface Validator { void validate(T value); /** - * Validate this setting against its dependencies, specified by {@link #settings()}. Default implementation is a no operation. + * Validate this setting against its dependencies, specified by {@link #settings()}. The default implementation does nothing, + * 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 @@ -841,9 +842,11 @@ default void validate(T value, Map, T> settings) { } /** - * The settings needed by this validator. Default value is an empty iterator. + * 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 + * other settings. * - * @return the settings needed to validate; these can be used for cross-settings validation + * @return the settings on which the validity of this setting depends. */ default Iterator> settings() { return Collections.emptyIterator();