Skip to content

Commit

Permalink
Correctly determine defaults of settings which depend on other settin…
Browse files Browse the repository at this point in the history
…gs (elastic#65989)

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.
  • Loading branch information
gwbrown committed Dec 8, 2020
1 parent 43fa468 commit 7457f61
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,14 @@ private T get(Settings settings, boolean validate) {
*/
public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) {
if (exists(source) == false) {
builder.put(getKey(), getRaw(defaultSettings));
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));
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Integer> dependedSetting = Setting.intSetting(dependedSettingName, 1, Property.Dynamic, Property.NodeScope);

final String dependentSettingName = "this.setting.depends.on.another";
Setting<Integer> 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<Integer> fooBarBaz = Setting.intSetting("foo.bar.baz", 1, Property.NodeScope);
Setting<Integer> fooBar = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope);
Expand Down

0 comments on commit 7457f61

Please sign in to comment.