From 081c1ad416e45592bee281df5bb5778787be2f8c Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Tue, 10 Jan 2017 15:14:55 +0100 Subject: [PATCH] Allow affix settings to delegate to actual settings (#22523) Affix settings are useful to namespace a certain setting. Yet, affix settings must be specialized for their concrete type which causes lot of code duplication. This commit allows to reuse an existing setting with and affix setting as soon as a concrete key is available. --- .../common/logging/ESLoggerFactory.java | 4 +- .../common/settings/Setting.java | 83 ++++++++++--------- .../common/settings/ScopedSettingsTests.java | 62 +++++++++++++- .../common/settings/SettingTests.java | 23 +++-- .../azure/storage/AzureStorageSettings.java | 16 ++-- 5 files changed, 132 insertions(+), 56 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java b/core/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java index 548c1da5a8c..16f47f78ddb 100644 --- a/core/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java +++ b/core/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java @@ -38,8 +38,8 @@ public final class ESLoggerFactory { public static final Setting LOG_DEFAULT_LEVEL_SETTING = new Setting<>("logger.level", Level.INFO.name(), Level::valueOf, Property.NodeScope); public static final Setting LOG_LEVEL_SETTING = - Setting.prefixKeySetting("logger.", Level.INFO.name(), Level::valueOf, - Property.Dynamic, Property.NodeScope); + Setting.prefixKeySetting("logger.", (key) -> new Setting<>(key, Level.INFO.name(), Level::valueOf, Property.Dynamic, + Property.NodeScope)); public static Logger getLogger(String prefix, String name) { return getLogger(prefix, LogManager.getLogger(name)); diff --git a/core/src/main/java/org/elasticsearch/common/settings/Setting.java b/core/src/main/java/org/elasticsearch/common/settings/Setting.java index 4ed64b77c4a..c4eb5a11213 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -42,12 +42,15 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.EnumSet; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Function; +import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -894,31 +897,22 @@ public class Setting extends ToXContentToBytes { * can easily be added with this setting. Yet, prefix key settings don't support updaters out of the box unless * {@link #getConcreteSetting(String)} is used to pull the updater. */ - public static Setting prefixKeySetting(String prefix, String defaultValue, Function parser, - Property... properties) { - return affixKeySetting(AffixKey.withPrefix(prefix), (s) -> defaultValue, parser, properties); + public static Setting prefixKeySetting(String prefix, Function> delegateFactory) { + return affixKeySetting(new AffixKey(prefix), delegateFactory); } /** * This setting type allows to validate settings that have the same type and a common prefix and suffix. For instance - * storage.${backend}.enable=[true|false] can easily be added with this setting. Yet, adfix key settings don't support updaters + * storage.${backend}.enable=[true|false] can easily be added with this setting. Yet, affix key settings don't support updaters * out of the box unless {@link #getConcreteSetting(String)} is used to pull the updater. */ - public static Setting affixKeySetting(String prefix, String suffix, Function defaultValue, - Function parser, Property... properties) { - return affixKeySetting(AffixKey.withAffix(prefix, suffix), defaultValue, parser, properties); + public static Setting affixKeySetting(String prefix, String suffix, Function> delegateFactory) { + return affixKeySetting(new AffixKey(prefix, suffix), delegateFactory); } - public static Setting affixKeySetting(String prefix, String suffix, String defaultValue, Function parser, - Property... properties) { - return affixKeySetting(prefix, suffix, (s) -> defaultValue, parser, properties); - } - - public static Setting affixKeySetting(AffixKey key, Function defaultValue, Function parser, - Property... properties) { - return new Setting(key, defaultValue, parser, properties) { - - @Override + private static Setting affixKeySetting(AffixKey key, Function> delegateFactory) { + Setting delegate = delegateFactory.apply("_na_"); + return new Setting(key, delegate.defaultValue, delegate.parser, delegate.properties.toArray(new Property[0])) { boolean isGroupSetting() { return true; } @@ -931,7 +925,7 @@ public class Setting extends ToXContentToBytes { @Override public Setting getConcreteSetting(String key) { if (match(key)) { - return new Setting<>(key, defaultValue, parser, properties); + return delegateFactory.apply(key); } else { throw new IllegalArgumentException("key [" + key + "] must match [" + getKey() + "] but didn't."); } @@ -939,14 +933,19 @@ public class Setting extends ToXContentToBytes { @Override public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { - for (Map.Entry entry : defaultSettings.getAsMap().entrySet()) { - if (match(entry.getKey())) { - getConcreteSetting(entry.getKey()).diff(builder, source, defaultSettings); + final Set concreteSettings = new HashSet<>(); + for (String settingKey : defaultSettings.getAsMap().keySet()) { + if (match(settingKey)) { + concreteSettings.add(key.getConcreteString(settingKey)); } } + for (String key : concreteSettings) { + getConcreteSetting(key).diff(builder, source, defaultSettings); + } } }; - } + }; + public interface Key { @@ -1013,36 +1012,44 @@ public class Setting extends ToXContentToBytes { } public static final class AffixKey implements Key { - public static AffixKey withPrefix(String prefix) { - return new AffixKey(prefix, null); - } - - public static AffixKey withAffix(String prefix, String suffix) { - return new AffixKey(prefix, suffix); - } - + private final Pattern pattern; private final String prefix; private final String suffix; - public AffixKey(String prefix, String suffix) { + AffixKey(String prefix) { + this(prefix, null); + } + + AffixKey(String prefix, String suffix) { assert prefix != null || suffix != null: "Either prefix or suffix must be non-null"; + this.prefix = prefix; if (prefix.endsWith(".") == false) { throw new IllegalArgumentException("prefix must end with a '.'"); } this.suffix = suffix; + if (suffix == null) { + pattern = Pattern.compile("(" + Pattern.quote(prefix) + "((?:[-\\w]+[.])*[-\\w]+$))"); + } else { + // the last part of this regexp is for lists since they are represented as x.${namespace}.y.1, x.${namespace}.y.2 + pattern = Pattern.compile("(" + Pattern.quote(prefix) + "\\w+\\." + Pattern.quote(suffix) + ")(?:\\.\\d+)?"); + } } @Override public boolean match(String key) { - boolean match = true; - if (prefix != null) { - match = key.startsWith(prefix); + return pattern.matcher(key).matches(); + } + + /** + * Returns a string representation of the concrete setting key + */ + String getConcreteString(String key) { + Matcher matcher = pattern.matcher(key); + if (matcher.matches() == false) { + throw new IllegalStateException("can't get concrete string for key " + key + " key doesn't match"); } - if (suffix != null) { - match = match && key.endsWith(suffix); - } - return match; + return matcher.group(1); } public SimpleKey toConcreteKey(String missingPart) { diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 851ea26a19d..1fbed949b5f 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -214,7 +214,8 @@ public class ScopedSettingsTests extends ESTestCase { Setting fooBarBaz = Setting.intSetting("foo.bar.baz", 1, Property.NodeScope); Setting fooBar = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope); Setting someGroup = Setting.groupSetting("some.group.", Property.Dynamic, Property.NodeScope); - Setting someAffix = Setting.affixKeySetting("some.prefix.", "somekey", "true", Boolean::parseBoolean, Property.NodeScope); + Setting someAffix = Setting.affixKeySetting("some.prefix.", "somekey", (key) -> Setting.boolSetting(key, true, + Property.NodeScope)); Setting> foorBarQuux = Setting.listSetting("foo.bar.quux", Arrays.asList("a", "b", "c"), Function.identity(), Property.NodeScope); ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(fooBar, fooBarBaz, foorBarQuux, @@ -253,6 +254,65 @@ public class ScopedSettingsTests extends ESTestCase { assertThat(diff.getAsInt("foo.bar", null), equalTo(1)); } + public void testDiffWithAffixAndComplexMatcher() { + Setting fooBarBaz = Setting.intSetting("foo.bar.baz", 1, Property.NodeScope); + Setting fooBar = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope); + Setting someGroup = Setting.groupSetting("some.group.", Property.Dynamic, Property.NodeScope); + Setting someAffix = Setting.affixKeySetting("some.prefix.", "somekey", (key) -> Setting.boolSetting(key, true, + Property.NodeScope)); + Setting> foorBarQuux = Setting.affixKeySetting("foo.", "quux", + (key) -> Setting.listSetting(key, Arrays.asList("a", "b", "c"), Function.identity(), Property.NodeScope)); + ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(fooBar, fooBarBaz, foorBarQuux, + someGroup, someAffix))); + Settings diff = settings.diff(Settings.builder().put("foo.bar", 5).build(), Settings.EMPTY); + assertEquals(1, diff.getAsMap().size()); + assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1)); + assertNull(diff.getAsArray("foo.bar.quux", null)); // affix settings don't know their concrete keys + + diff = settings.diff( + Settings.builder().put("foo.bar", 5).build(), + Settings.builder().put("foo.bar.baz", 17).putArray("foo.bar.quux", "d", "e", "f").build()); + assertEquals(4, diff.getAsMap().size()); + assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(17)); + assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"d", "e", "f"}); + + diff = settings.diff( + Settings.builder().put("some.group.foo", 5).build(), + Settings.builder().put("some.group.foobar", 17, "some.group.foo", 25).build()); + assertEquals(3, diff.getAsMap().size()); + assertThat(diff.getAsInt("some.group.foobar", null), equalTo(17)); + assertNull(diff.get("some.group.foo")); + assertNull(diff.getAsArray("foo.bar.quux", null)); // affix settings don't know their concrete keys + assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1)); + assertThat(diff.getAsInt("foo.bar", null), equalTo(1)); + + diff = settings.diff( + Settings.builder().put("some.prefix.foo.somekey", 5).build(), + Settings.builder().put("some.prefix.foobar.somekey", 17, + "some.prefix.foo.somekey", 18).build()); + assertEquals(3, diff.getAsMap().size()); + assertThat(diff.getAsInt("some.prefix.foobar.somekey", null), equalTo(17)); + assertNull(diff.get("some.prefix.foo.somekey")); + assertNull(diff.getAsArray("foo.bar.quux", null)); // affix settings don't know their concrete keys + assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1)); + assertThat(diff.getAsInt("foo.bar", null), equalTo(1)); + + diff = settings.diff( + Settings.builder().put("some.prefix.foo.somekey", 5).build(), + Settings.builder().put("some.prefix.foobar.somekey", 17, + "some.prefix.foo.somekey", 18) + .putArray("foo.bar.quux", "x", "y", "z") + .putArray("foo.baz.quux", "d", "e", "f") + .build()); + assertEquals(9, diff.getAsMap().size()); + assertThat(diff.getAsInt("some.prefix.foobar.somekey", null), equalTo(17)); + assertNull(diff.get("some.prefix.foo.somekey")); + assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"x", "y", "z"}); + assertArrayEquals(diff.getAsArray("foo.baz.quux", null), new String[] {"d", "e", "f"}); + assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1)); + assertThat(diff.getAsInt("foo.bar", null), equalTo(1)); + } + public void testUpdateTracer() { ClusterSettings settings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); AtomicReference> ref = new AtomicReference<>(); diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 2bd5dea3c10..4ce23ebcaf0 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -426,7 +426,7 @@ public class SettingTests extends ESTestCase { } public void testDynamicKeySetting() { - Setting setting = Setting.prefixKeySetting("foo.", "false", Boolean::parseBoolean, Property.NodeScope); + Setting setting = Setting.prefixKeySetting("foo.", (key) -> Setting.boolSetting(key, false, Property.NodeScope)); assertTrue(setting.hasComplexMatcher()); assertTrue(setting.match("foo.bar")); assertFalse(setting.match("foo")); @@ -444,11 +444,11 @@ public class SettingTests extends ESTestCase { public void testAffixKeySetting() { Setting setting = - Setting.affixKeySetting("foo.", "enable", "false", Boolean::parseBoolean, Property.NodeScope); + Setting.affixKeySetting("foo.", "enable", (key) -> Setting.boolSetting(key, false, Property.NodeScope)); assertTrue(setting.hasComplexMatcher()); assertTrue(setting.match("foo.bar.enable")); assertTrue(setting.match("foo.baz.enable")); - assertTrue(setting.match("foo.bar.baz.enable")); + assertFalse(setting.match("foo.bar.baz.enable")); assertFalse(setting.match("foo.bar")); assertFalse(setting.match("foo.bar.baz.enabled")); assertFalse(setting.match("foo")); @@ -459,11 +459,23 @@ public class SettingTests extends ESTestCase { IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> setting.getConcreteSetting("foo")); assertEquals("key [foo] must match [foo.*.enable] but didn't.", exc.getMessage()); - exc = expectThrows(IllegalArgumentException.class, () -> Setting.affixKeySetting("foo", "enable", "false", - Boolean::parseBoolean, Property.NodeScope)); + exc = expectThrows(IllegalArgumentException.class, () -> Setting.affixKeySetting("foo", "enable", + (key) -> Setting.boolSetting(key, false, Property.NodeScope))); assertEquals("prefix must end with a '.'", exc.getMessage()); + + Setting> listAffixSetting = Setting.affixKeySetting("foo.", "bar", + (key) -> Setting.listSetting(key, Collections.emptyList(), Function.identity(), Property.NodeScope)); + + assertTrue(listAffixSetting.hasComplexMatcher()); + assertTrue(listAffixSetting.match("foo.test.bar")); + assertTrue(listAffixSetting.match("foo.test_1.bar")); + assertFalse(listAffixSetting.match("foo.buzz.baz.bar")); + assertFalse(listAffixSetting.match("foo.bar")); + assertFalse(listAffixSetting.match("foo.baz")); + assertFalse(listAffixSetting.match("foo")); } + public void testMinMaxInt() { Setting integerSetting = Setting.intSetting("foo.bar", 1, 0, 10, Property.NodeScope); try { @@ -530,4 +542,5 @@ public class SettingTests extends ESTestCase { assertThat(setting.get(Settings.builder().put("foo", "12h").build()), equalTo(TimeValue.timeValueHours(12))); assertThat(setting.get(Settings.EMPTY).getMillis(), equalTo(random.getMillis() * factor)); } + } diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java index 9d67eea628b..5e0de46f65a 100644 --- a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java +++ b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java @@ -25,6 +25,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.node.Node; import java.util.ArrayList; import java.util.Collections; @@ -34,19 +35,14 @@ import java.util.Map; import java.util.function.Function; public final class AzureStorageSettings { - private static final Setting.AffixKey TIMEOUT_KEY = Setting.AffixKey.withAffix(Storage.PREFIX, "timeout"); - - private static final Setting TIMEOUT_SETTING = Setting.affixKeySetting( - TIMEOUT_KEY, - (s) -> Storage.TIMEOUT_SETTING.get(s).toString(), - (s) -> Setting.parseTimeValue(s, TimeValue.timeValueSeconds(-1), TIMEOUT_KEY.toString()), - Setting.Property.NodeScope); + private static final Setting TIMEOUT_SETTING = Setting.affixKeySetting(Storage.PREFIX, "timeout", + (key) -> Setting.timeSetting(key, Storage.TIMEOUT_SETTING, Setting.Property.NodeScope)); private static final Setting ACCOUNT_SETTING = - Setting.affixKeySetting(Storage.PREFIX, "account", "", Function.identity(), Setting.Property.NodeScope); + Setting.affixKeySetting(Storage.PREFIX, "account", (key) -> Setting.simpleString(key, Setting.Property.NodeScope)); private static final Setting KEY_SETTING = - Setting.affixKeySetting(Storage.PREFIX, "key", "", Function.identity(), Setting.Property.NodeScope); + Setting.affixKeySetting(Storage.PREFIX, "key", (key) -> Setting.simpleString(key, Setting.Property.NodeScope)); private static final Setting DEFAULT_SETTING = - Setting.affixKeySetting(Storage.PREFIX, "default", "false", Boolean::valueOf, Setting.Property.NodeScope); + Setting.affixKeySetting(Storage.PREFIX, "default", (key) -> Setting.boolSetting(key, false, Setting.Property.NodeScope)); private final String name;