honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Don't omit default values when updating routing exclusions (#33638) 

Exclusion setting `cluster.routing.allocation.exclude._host` default value
is an empty string.

When an exclusion setting is sent with a null value the
o.e.c.s.Setting#innerGetRaw API return an empty string (probably to
avoid a NullPointerException to be raised).

The o.e.c.r.a.d.FilterAllocationDecider class is developed to omit
updates of default values for exclusion setting.

That's why a null exclusion setting value is translated to an empty
string which is equals to the exclusion default value which is
configured to be ignored.

A simple fix would be to not omit default values for exclusion setting
and keep the NullPointerException guard. This is the purpose of this
commit.

Closes #32721
This commit is contained in:
Christophe Bismuth 2018-10-19 13:57:41 +02:00 committed by Simon Willnauer
parent aeb3cda585
commit 3036ab1048
5 changed files with 52 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java
View File

@ -101,9 +101,9 @@ public class FilterAllocationDecider extends AllocationDecider {
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

6
server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
View File

@ -301,13 +301,13 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
* 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 <T> void addAffixMapUpdateConsumer(Setting.AffixSetting<T> setting, Consumer<Map<String, T>> consumer,
BiConsumer<String, T> validator, boolean omitDefaults) {
public synchronized <T> void addAffixMapUpdateConsumer(Setting.AffixSetting<T> setting, Consumer<Map<String, T>> consumer,
BiConsumer<String, T> 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) {

6
server/src/main/java/org/elasticsearch/common/settings/Setting.java
View File

@ -700,7 +700,7 @@ public class Setting<T> implements ToXContentObject {
}
AbstractScopedSettings.SettingUpdater<Map<String, T>> newAffixMapUpdater(Consumer<Map<String, T>> consumer, Logger logger,
BiConsumer<String, T> validator, boolean omitDefaults) {
BiConsumer<String, T> validator) {
return new AbstractScopedSettings.SettingUpdater<Map<String, T>>() {
@Override
@ -721,9 +721,7 @@ public class Setting<T> implements ToXContentObject {
// 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;

22
server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
View File

@ -371,9 +371,8 @@ public class ScopedSettingsTests extends ESTestCase {
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 class ScopedSettingsTests extends ESTestCase {
assertEquals(2, listResults.size());
assertEquals(2, intResults.size());
listResults.clear();
intResults.clear();
@ -416,17 +414,9 @@ public class ScopedSettingsTests extends ESTestCase {
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 class ScopedSettingsTests extends ESTestCase {
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)

38
server/src/test/java/org/elasticsearch/common/settings/SettingTests.java
View File

@ -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.List;
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 class SettingTests extends ESTestCase {
}
}
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<String> affixSetting =
Setting.prefixKeySetting("prefix" + ".",
(key) -> Setting.simpleString(key, (value, map) -> {}, Property.Dynamic, Property.NodeScope));
final Consumer<Map<String, String>> consumer = (map) -> {};
final BiConsumer<String, String> validator = (s1, s2) -> {};
// WHEN creating an affix updater
final SettingUpdater<Map<String, String>> 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<String, String> 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);
}
}