From b13571211ed624dd3a2a956d55e39f1e57b36a20 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Wed, 24 Jan 2018 09:47:17 +0100 Subject: [PATCH] Settings: Introduce settings updater for a list of settings (#28338) This introduces a settings updater that allows to specify a list of settings. Whenever one of those settings changes, the whole block of settings is passed to the consumer. This also fixes an issue with affix settings, when used in combination with group settings, which could result in no found settings when used to get a setting for a namespace. Lastly logging has been slightly changed, so that filtered settings now only log the setting key. Another bug has been fixed for the mock log appender, which did not work, when checking for the exact message. Closes #28047 --- .../settings/AbstractScopedSettings.java | 10 +++ .../common/settings/Setting.java | 66 +++++++++++++--- .../common/settings/SettingTests.java | 76 +++++++++++++++++++ .../common/settings/SettingsFilterTests.java | 44 ++++++++++- .../elasticsearch/test/MockLogAppender.java | 2 +- 5 files changed, 187 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 e2f4d7697b62d..c3c6de5355af4 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -194,6 +194,16 @@ public synchronized void addSettingsUpdateConsumer(Setting setting, Consu addSettingsUpdater(setting.newUpdater(consumer, logger, validator)); } + /** + * Adds a settings consumer that is only executed if any setting in the supplied list of settings is changed. In that case all the + * settings are specified in the argument are returned. + * + * Also automatically adds empty consumers for all settings in order to activate logging + */ + public synchronized void addSettingsUpdateConsumer(Consumer consumer, List> settings) { + addSettingsUpdater(Setting.groupedSettingsUpdater(consumer, logger, settings)); + } + /** * Adds a settings consumer for affix settings. Affix settings have a namespace associated to it that needs to be available to the * consumer in order to be processed correctly. 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 fd91a8a7601c6..f7f67e424cc8d 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -509,10 +509,10 @@ public Tuple getValue(Settings current, Settings previous) { @Override public void apply(Tuple value, Settings current, Settings previous) { if (aSettingUpdater.hasChanged(current, previous)) { - logger.info("updating [{}] from [{}] to [{}]", aSetting.key, aSetting.getRaw(previous), aSetting.getRaw(current)); + logSettingUpdate(aSetting, current, previous, logger); } if (bSettingUpdater.hasChanged(current, previous)) { - logger.info("updating [{}] from [{}] to [{}]", bSetting.key, bSetting.getRaw(previous), bSetting.getRaw(current)); + logSettingUpdate(bSetting, current, previous, logger); } consumer.accept(value.v1(), value.v2()); } @@ -524,6 +524,46 @@ public String toString() { }; } + static AbstractScopedSettings.SettingUpdater groupedSettingsUpdater(Consumer consumer, Logger logger, + final List> configuredSettings) { + + return new AbstractScopedSettings.SettingUpdater() { + + private Settings get(Settings settings) { + return settings.filter(s -> { + for (Setting setting : configuredSettings) { + if (setting.key.match(s)) { + return true; + } + } + return false; + }); + } + + @Override + public boolean hasChanged(Settings current, Settings previous) { + Settings currentSettings = get(current); + Settings previousSettings = get(previous); + return currentSettings.equals(previousSettings) == false; + } + + @Override + public Settings getValue(Settings current, Settings previous) { + return get(current); + } + + @Override + public void apply(Settings value, Settings current, Settings previous) { + consumer.accept(value); + } + + @Override + public String toString() { + return "Updater grouped: " + configuredSettings.stream().map(Setting::getKey).collect(Collectors.joining(", ")); + } + }; + } + public static class AffixSetting extends Setting { private final AffixKey key; private final Function> delegateFactory; @@ -541,7 +581,7 @@ boolean isGroupSetting() { } private Stream matchStream(Settings settings) { - return settings.keySet().stream().filter((key) -> match(key)).map(settingKey -> key.getConcreteString(settingKey)); + return settings.keySet().stream().filter(this::match).map(key::getConcreteString); } public Set getSettingsDependencies(String settingsKey) { @@ -812,9 +852,7 @@ public Settings getValue(Settings current, Settings previous) { @Override public void apply(Settings value, Settings current, Settings previous) { - if (logger.isInfoEnabled()) { // getRaw can create quite some objects - logger.info("updating [{}] from [{}] to [{}]", key, getRaw(previous), getRaw(current)); - } + Setting.logSettingUpdate(GroupSetting.this, current, previous, logger); consumer.accept(value); } @@ -902,7 +940,7 @@ public T getValue(Settings current, Settings previous) { @Override public void apply(T value, Settings current, Settings previous) { - logger.info("updating [{}] from [{}] to [{}]", key, getRaw(previous), getRaw(current)); + logSettingUpdate(Setting.this, current, previous, logger); consumer.accept(value); } } @@ -1138,6 +1176,16 @@ private static String arrayToParsableString(List array) { } } + static void logSettingUpdate(Setting setting, Settings current, Settings previous, Logger logger) { + if (logger.isInfoEnabled()) { + if (setting.isFiltered()) { + logger.info("updating [{}]", setting.key); + } else { + logger.info("updating [{}] from [{}] to [{}]", setting.key, setting.getRaw(previous), setting.getRaw(current)); + } + } + } + public static Setting groupSetting(String key, Property... properties) { return groupSetting(key, (s) -> {}, properties); } @@ -1308,8 +1356,8 @@ public static final class AffixKey implements Key { 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+)?"); + // the last part of this regexp is to support both list and group keys + pattern = Pattern.compile("(" + Pattern.quote(prefix) + "([-\\w]+)\\." + Pattern.quote(suffix) + ")(?:\\..*)?"); } } 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 4a4beb2e0e3ef..180f11730dfed 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -38,6 +38,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; @@ -712,4 +713,79 @@ public void testTimeValue() { assertThat(setting.get(Settings.EMPTY).getMillis(), equalTo(random.getMillis() * factor)); } + public void testSettingsGroupUpdater() { + Setting intSetting = Setting.intSetting("prefix.foo", 1, Property.NodeScope, Property.Dynamic); + Setting intSetting2 = Setting.intSetting("prefix.same", 1, Property.NodeScope, Property.Dynamic); + AbstractScopedSettings.SettingUpdater updater = Setting.groupedSettingsUpdater(s -> {}, logger, + Arrays.asList(intSetting, intSetting2)); + + Settings current = Settings.builder().put("prefix.foo", 123).put("prefix.same", 5555).build(); + Settings previous = Settings.builder().put("prefix.foo", 321).put("prefix.same", 5555).build(); + assertTrue(updater.apply(current, previous)); + } + + public void testSettingsGroupUpdaterRemoval() { + Setting intSetting = Setting.intSetting("prefix.foo", 1, Property.NodeScope, Property.Dynamic); + Setting intSetting2 = Setting.intSetting("prefix.same", 1, Property.NodeScope, Property.Dynamic); + AbstractScopedSettings.SettingUpdater updater = Setting.groupedSettingsUpdater(s -> {}, logger, + Arrays.asList(intSetting, intSetting2)); + + Settings current = Settings.builder().put("prefix.same", 5555).build(); + Settings previous = Settings.builder().put("prefix.foo", 321).put("prefix.same", 5555).build(); + assertTrue(updater.apply(current, previous)); + } + + public void testSettingsGroupUpdaterWithAffixSetting() { + Setting intSetting = Setting.intSetting("prefix.foo", 1, Property.NodeScope, Property.Dynamic); + Setting.AffixSetting prefixKeySetting = + Setting.prefixKeySetting("prefix.foo.bar.", key -> Setting.simpleString(key, Property.NodeScope, Property.Dynamic)); + Setting.AffixSetting affixSetting = + Setting.affixKeySetting("prefix.foo.", "suffix", key -> Setting.simpleString(key,Property.NodeScope, Property.Dynamic)); + + AbstractScopedSettings.SettingUpdater updater = Setting.groupedSettingsUpdater(s -> {}, logger, + Arrays.asList(intSetting, prefixKeySetting, affixSetting)); + + Settings.Builder currentSettingsBuilder = Settings.builder() + .put("prefix.foo.bar.baz", "foo") + .put("prefix.foo.infix.suffix", "foo"); + Settings.Builder previousSettingsBuilder = Settings.builder() + .put("prefix.foo.bar.baz", "foo") + .put("prefix.foo.infix.suffix", "foo"); + boolean removePrefixKeySetting = randomBoolean(); + boolean changePrefixKeySetting = randomBoolean(); + boolean removeAffixKeySetting = randomBoolean(); + boolean changeAffixKeySetting = randomBoolean(); + boolean removeAffixNamespace = randomBoolean(); + + if (removePrefixKeySetting) { + previousSettingsBuilder.remove("prefix.foo.bar.baz"); + } + if (changePrefixKeySetting) { + currentSettingsBuilder.put("prefix.foo.bar.baz", "bar"); + } + if (removeAffixKeySetting) { + previousSettingsBuilder.remove("prefix.foo.infix.suffix"); + } + if (changeAffixKeySetting) { + currentSettingsBuilder.put("prefix.foo.infix.suffix", "bar"); + } + if (removeAffixKeySetting == false && changeAffixKeySetting == false && removeAffixNamespace) { + currentSettingsBuilder.remove("prefix.foo.infix.suffix"); + currentSettingsBuilder.put("prefix.foo.infix2.suffix", "bar"); + previousSettingsBuilder.put("prefix.foo.infix2.suffix", "bar"); + } + + boolean expectedChange = removeAffixKeySetting || removePrefixKeySetting || changeAffixKeySetting || changePrefixKeySetting + || removeAffixNamespace; + assertThat(updater.apply(currentSettingsBuilder.build(), previousSettingsBuilder.build()), is(expectedChange)); + } + + public void testAffixNamespacesWithGroupSetting() { + final Setting.AffixSetting affixSetting = + Setting.affixKeySetting("prefix.","suffix", + (key) -> Setting.groupSetting(key + ".", Setting.Property.Dynamic, Setting.Property.NodeScope)); + + assertThat(affixSetting.getNamespaces(Settings.builder().put("prefix.infix.suffix", "anything").build()), hasSize(1)); + assertThat(affixSetting.getNamespaces(Settings.builder().put("prefix.infix.suffix.anything", "anything").build()), hasSize(1)); + } } diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java index 9e6d4be7095f0..dfece2d9d459c 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java @@ -18,16 +18,22 @@ */ package org.elasticsearch.common.settings; -import org.elasticsearch.common.Strings; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.common.logging.Loggers; +import org.elasticsearch.common.logging.ServerLoggers; +import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.MockLogAppender; import org.elasticsearch.test.rest.FakeRestRequest; import java.io.IOException; import java.util.Arrays; import java.util.HashSet; +import java.util.function.Consumer; import static org.hamcrest.CoreMatchers.equalTo; @@ -100,7 +106,43 @@ public void testSettingsFiltering() throws IOException { .build(), "a.b.*.d" ); + } + + public void testFilteredSettingIsNotLogged() throws Exception { + Settings oldSettings = Settings.builder().put("key", "old").build(); + Settings newSettings = Settings.builder().put("key", "new").build(); + + Setting filteredSetting = Setting.simpleString("key", Property.Filtered); + assertExpectedLogMessages((testLogger) -> Setting.logSettingUpdate(filteredSetting, newSettings, oldSettings, testLogger), + new MockLogAppender.SeenEventExpectation("secure logging", "org.elasticsearch.test", Level.INFO, "updating [key]"), + new MockLogAppender.UnseenEventExpectation("unwanted old setting name", "org.elasticsearch.test", Level.INFO, "*old*"), + new MockLogAppender.UnseenEventExpectation("unwanted new setting name", "org.elasticsearch.test", Level.INFO, "*new*") + ); + } + + public void testRegularSettingUpdateIsFullyLogged() throws Exception { + Settings oldSettings = Settings.builder().put("key", "old").build(); + Settings newSettings = Settings.builder().put("key", "new").build(); + + Setting regularSetting = Setting.simpleString("key"); + assertExpectedLogMessages((testLogger) -> Setting.logSettingUpdate(regularSetting, newSettings, oldSettings, testLogger), + new MockLogAppender.SeenEventExpectation("regular logging", "org.elasticsearch.test", Level.INFO, + "updating [key] from [old] to [new]")); + } + private void assertExpectedLogMessages(Consumer consumer, + MockLogAppender.LoggingExpectation ... expectations) throws IllegalAccessException { + Logger testLogger = Loggers.getLogger("org.elasticsearch.test"); + MockLogAppender appender = new MockLogAppender(); + ServerLoggers.addAppender(testLogger, appender); + try { + appender.start(); + Arrays.stream(expectations).forEach(appender::addExpectation); + consumer.accept(testLogger); + appender.assertAllExpectationsMatched(); + } finally { + ServerLoggers.removeAppender(testLogger, appender); + } } private void testFiltering(Settings source, Settings filtered, String... patterns) throws IOException { diff --git a/test/framework/src/main/java/org/elasticsearch/test/MockLogAppender.java b/test/framework/src/main/java/org/elasticsearch/test/MockLogAppender.java index b35dc9563ce5c..6e5f919f33fdf 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/MockLogAppender.java +++ b/test/framework/src/main/java/org/elasticsearch/test/MockLogAppender.java @@ -92,7 +92,7 @@ public void match(LogEvent event) { saw = true; } } else { - if (event.getMessage().toString().contains(message)) { + if (event.getMessage().getFormattedMessage().contains(message)) { saw = true; } }