From beafc566a85571ba6d1ea199d7cbc5f883df09c8 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Mon, 7 Dec 2020 16:43:32 -0700 Subject: [PATCH 1/3] Correctly determine defaults of settings which depend on other settings This commit adjusts the behavior when calculating the diff between two `AbstractScopedSettings` objects, so that the default values of settings whose default values depend on the values of other settings are correctly calculated. Previously, when calculating the diff, the default value of a depended setting would be calculated based on the default value of the setting(s) it depends on, rather than the current value of those settings. --- .../common/settings/Setting.java | 6 ++++- .../common/settings/ScopedSettingsTests.java | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+), 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 28b7e56c2489f..dd372c48fe765 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -486,7 +486,11 @@ private T get(Settings settings, boolean validate) { * @param defaultSettings the default settings object to diff against */ public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { - if (exists(source) == false) { + if (exists(source) == false && exists(defaultSettings) == false) { + // If the setting is in neither source nor default, get the value from source as it may depend on the value of other settings + builder.put(getKey(), getRaw(source)); + } else if (exists(source) == false && exists(defaultSettings)) { + // If the setting is only in the defaults, use the value from the defaults builder.put(getKey(), getRaw(defaultSettings)); } } 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 ffe9842b3405c..4eea0ae2b0f85 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -737,6 +737,28 @@ public void testDiff() throws IOException { assertThat(diff.getAsInt("foo.bar", null), equalTo(1)); } + public void testDiffWithDependentSettings() { + final String dependedSettingName = "this.setting.is.depended.on"; + Setting dependedSetting = Setting.intSetting(dependedSettingName, 1, Property.Dynamic, Property.NodeScope); + + final String dependentSettingName = "this.setting.depends.on.another"; + Setting dependentSetting = new Setting<>(dependentSettingName, + (s) -> Integer.toString(dependedSetting.get(s) + 10), + (s) -> Setting.parseInt(s, 1, dependentSettingName), + Property.Dynamic, Property.NodeScope); + + ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(dependedSetting, dependentSetting))); + + // Ensure that the value of the dependent setting is correctly calculated based on the "source" settings + Settings diff = settings.diff(Settings.builder().put(dependedSettingName, 2).build(), Settings.EMPTY); + assertThat(diff.getAsInt(dependentSettingName, null), equalTo(12)); + + // Ensure that the value is correctly calculated if neither is set + diff = settings.diff(Settings.EMPTY, Settings.EMPTY); + assertThat(diff.getAsInt(dependedSettingName, null), equalTo(1)); + assertThat(diff.getAsInt(dependentSettingName, null), equalTo(11)); + } + public void testDiffWithAffixAndComplexMatcher() { Setting fooBarBaz = Setting.intSetting("foo.bar.baz", 1, Property.NodeScope); Setting fooBar = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope); From d0c6578d898a357ca2f4f6969f100359389bdb8c Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 8 Dec 2020 10:50:37 -0700 Subject: [PATCH 2/3] Rearrage `if` per review feedback --- .../elasticsearch/common/settings/Setting.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 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 dd372c48fe765..d71f53e0f24ec 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -486,12 +486,15 @@ private T get(Settings settings, boolean validate) { * @param defaultSettings the default settings object to diff against */ public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { - if (exists(source) == false && exists(defaultSettings) == false) { - // If the setting is in neither source nor default, get the value from source as it may depend on the value of other settings - builder.put(getKey(), getRaw(source)); - } else if (exists(source) == false && exists(defaultSettings)) { - // If the setting is only in the defaults, use the value from the defaults - builder.put(getKey(), getRaw(defaultSettings)); + if (exists(source) == false) { + if (exists(defaultSettings) == false) { + // If the setting is in neither `source` nor `default`, get the value + // from `source` as it may depend on the value of other settings + builder.put(getKey(), getRaw(source)); + } else { + // If the setting is only in the defaults, use the value from the defaults + builder.put(getKey(), getRaw(defaultSettings)); + } } } From a03aa53ac99d87326e9a10885ae95b6128ba020a Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 8 Dec 2020 10:56:31 -0700 Subject: [PATCH 3/3] Rearrage `if` per review feedback, actually this time --- .../java/org/elasticsearch/common/settings/Setting.java | 8 ++++---- 1 file changed, 4 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 d71f53e0f24ec..98f8b7b64c60a 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -487,13 +487,13 @@ private T get(Settings settings, boolean validate) { */ public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { if (exists(source) == false) { - if (exists(defaultSettings) == false) { + if (exists(defaultSettings)) { + // If the setting is only in the defaults, use the value from the defaults + builder.put(getKey(), getRaw(defaultSettings)); + } else { // If the setting is in neither `source` nor `default`, get the value // from `source` as it may depend on the value of other settings builder.put(getKey(), getRaw(source)); - } else { - // If the setting is only in the defaults, use the value from the defaults - builder.put(getKey(), getRaw(defaultSettings)); } } }