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..ed2d5384fa704 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 @@ -101,9 +101,9 @@ public FilterAllocationDecider(Settings settings, ClusterSettings clusterSetting setClusterRequireFilters(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING.getAsMap(settings)); setClusterExcludeFilters(CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING.getAsMap(settings)); setClusterIncludeFilters(CLUSTER_ROUTING_INCLUDE_GROUP_SETTING.getAsMap(settings)); - clusterSettings.addAffixMapUpdateConsumer(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING, this::setClusterRequireFilters, (a,b)-> {}, true); - clusterSettings.addAffixMapUpdateConsumer(CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING, this::setClusterExcludeFilters, (a,b)-> {}, true); - clusterSettings.addAffixMapUpdateConsumer(CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, this::setClusterIncludeFilters, (a,b)-> {}, true); + clusterSettings.addAffixMapUpdateConsumer(CLUSTER_ROUTING_REQUIRE_GROUP_SETTING, this::setClusterRequireFilters, (a, b) -> {}); + clusterSettings.addAffixMapUpdateConsumer(CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING, this::setClusterExcludeFilters, (a, b) -> {}); + clusterSettings.addAffixMapUpdateConsumer(CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, this::setClusterIncludeFilters, (a, b) -> {}); } @Override 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..ab635c1d1c7c5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -301,13 +301,13 @@ private void ensureSettingIsRegistered(Setting.AffixSetting setting) { * consumer in order to be processed correctly. This consumer will get a namespace to value map instead of each individual namespace * and value as in {@link #addAffixUpdateConsumer(Setting.AffixSetting, BiConsumer, BiConsumer)} */ - public synchronized void addAffixMapUpdateConsumer(Setting.AffixSetting setting, Consumer> consumer, - BiConsumer validator, boolean omitDefaults) { + public synchronized void addAffixMapUpdateConsumer(Setting.AffixSetting setting, Consumer> consumer, + BiConsumer validator) { final Setting registeredSetting = this.complexMatchers.get(setting.getKey()); if (setting != registeredSetting) { throw new IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]"); } - addSettingsUpdater(setting.newAffixMapUpdater(consumer, logger, validator, omitDefaults)); + addSettingsUpdater(setting.newAffixMapUpdater(consumer, logger, validator)); } synchronized void addSettingsUpdater(SettingUpdater updater) { 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..8a0fb852ec9d7 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -700,7 +700,7 @@ public void apply(Map, T> value, Settin } AbstractScopedSettings.SettingUpdater> newAffixMapUpdater(Consumer> consumer, Logger logger, - BiConsumer validator, boolean omitDefaults) { + BiConsumer validator) { return new AbstractScopedSettings.SettingUpdater>() { @Override @@ -721,9 +721,7 @@ public Map getValue(Settings current, Settings previous) { // only the ones that have changed otherwise we might get too many updates // the hasChanged above checks only if there are any changes T value = updater.getValue(current, previous); - if ((omitDefaults && value.equals(concreteSetting.getDefault(current))) == false) { - result.put(namespace, value); - } + result.put(namespace, value); } }); return result; 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..1846e180ad6f9 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -371,9 +371,8 @@ public void testAddConsumerAffixMap() { listResults.clear(); listResults.putAll(map); }; - boolean omitDefaults = randomBoolean(); - service.addAffixMapUpdateConsumer(listSetting, listConsumer, (s, k) -> {}, omitDefaults); - service.addAffixMapUpdateConsumer(intSetting, intConsumer, (s, k) -> {}, omitDefaults); + service.addAffixMapUpdateConsumer(listSetting, listConsumer, (s, k) -> {}); + service.addAffixMapUpdateConsumer(intSetting, intConsumer, (s, k) -> {}); assertEquals(0, listResults.size()); assertEquals(0, intResults.size()); service.applySettings(Settings.builder() @@ -403,7 +402,6 @@ public void testAddConsumerAffixMap() { assertEquals(2, listResults.size()); assertEquals(2, intResults.size()); - listResults.clear(); intResults.clear(); @@ -416,17 +414,9 @@ public void testAddConsumerAffixMap() { assertNull("test wasn't changed", intResults.get("test")); assertEquals(8, intResults.get("test_1").intValue()); assertNull("test_list wasn't changed", listResults.get("test_list")); - if (omitDefaults) { - assertNull(listResults.get("test_list_1")); - assertFalse(listResults.containsKey("test_list_1")); - assertEquals(0, listResults.size()); - assertEquals(1, intResults.size()); - } else { - assertEquals(Arrays.asList(1), listResults.get("test_list_1")); // reset to default - assertEquals(1, listResults.size()); - assertEquals(1, intResults.size()); - } - + assertEquals(Arrays.asList(1), listResults.get("test_list_1")); // reset to default + assertEquals(1, listResults.size()); + assertEquals(1, intResults.size()); } public void testAffixMapConsumerNotCalledWithNull() { @@ -442,7 +432,7 @@ public void testAffixMapConsumerNotCalledWithNull() { affixResults.clear(); affixResults.putAll(map); }; - service.addAffixMapUpdateConsumer(prefixSetting, consumer, (s, k) -> {}, randomBoolean()); + service.addAffixMapUpdateConsumer(prefixSetting, consumer, (s, k) -> {}); assertEquals(0, affixResults.size()); service.applySettings(Settings.builder() .put("eggplant._name", 2) 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..08c6b5a31fb21 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.settings; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.settings.AbstractScopedSettings.SettingUpdater; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; @@ -33,6 +34,8 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -890,4 +893,39 @@ public void testExistsWithFallback() { } } + public void testAffixMapUpdateWithNullSettingValue() { + // GIVEN an affix setting changed from "prefix._foo"="bar" to "prefix._foo"=null + final Settings current = Settings.builder() + .put("prefix._foo", (String) null) + .build(); + + final Settings previous = Settings.builder() + .put("prefix._foo", "bar") + .build(); + + final Setting.AffixSetting affixSetting = + Setting.prefixKeySetting("prefix" + ".", + (key) -> Setting.simpleString(key, (value, map) -> {}, Property.Dynamic, Property.NodeScope)); + + final Consumer> consumer = (map) -> {}; + final BiConsumer validator = (s1, s2) -> {}; + + // WHEN creating an affix updater + final SettingUpdater> updater = affixSetting.newAffixMapUpdater(consumer, logger, validator); + + // THEN affix updater is always expected to have changed (even when defaults are omitted) + assertTrue(updater.hasChanged(current, previous)); + + // THEN changes are expected when defaults aren't omitted + final Map updatedSettings = updater.getValue(current, previous); + assertNotNull(updatedSettings); + assertEquals(1, updatedSettings.size()); + + // THEN changes are reported when defaults aren't omitted + final String key = updatedSettings.keySet().iterator().next(); + final String value = updatedSettings.get(key); + assertEquals("_foo", key); + assertEquals("", value); + } + }