-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include fallback settings when checking dependencies #33522
Include fallback settings when checking dependencies #33522
Conversation
Today when checking settings dependencies, we do not check if fallback settings are present. This means, for example, that if cluster.remote.*.seeds falls back to search.remote.*.seeds, and cluster.remote.*.skip_unavailable and search.remote.*.skip_unavailable depend on cluster.remote.*.seeds, and we have set search.remote.*.seeds and search.remote.*.skip_unavailable, then validation will fail because it is expected that cluster.ermote.*.seeds is set here. This commit addresses this by also checking fallback settings when validating dependencies. To do this, we adjust the settings exist method to also check for fallback settings, a case that it was not handling previously.
Pinging @elastic/es-core-infra |
@@ -461,16 +462,19 @@ void validate( | |||
} | |||
throw new IllegalArgumentException(msg); | |||
} else { | |||
Set<String> settingsDependencies = setting.getSettingsDependencies(key); | |||
Set<Setting<?>> settingsDependencies = setting.getSettingsDependencies(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is made so that we can validate dependencies exist using the actual settings object, and can therefore use fallback settings when doing the validation, which would not be possible if using the top-level dependent key only.
throw new IllegalArgumentException("Missing required setting [" | ||
+ requiredSetting + "] for setting [" + setting.getKey() + "]"); | ||
for (final Setting<?> settingDependency : settingsDependencies) { | ||
if (settingDependency.exists(settings) == false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we change from matching the key to using exists, so that we can use fallback settings if needed.
public boolean exists(Settings settings) { | ||
return settings.keySet().contains(getKey()); | ||
public boolean exists(final Settings settings) { | ||
Setting<?> current = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was not checking fallback settings, so we could get false here if the fallback setting was set but not the actual setting.
@@ -914,40 +921,6 @@ public String toString() { | |||
} | |||
} | |||
|
|||
private static class ListSetting<T> extends Setting<List<T>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this code so that it's closer to where ListSetting
s are used.
|
||
private ListSetting( | ||
final String key, | ||
final @Nullable Setting<List<T>> fallbackSetting, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallback settings were not being pushed down here, so ListSetting
s did not know until this change that they could have fallback settings.
if (defaultStringValue.apply(Settings.EMPTY) == null) { | ||
throw new IllegalArgumentException("default value function must not return null"); | ||
} | ||
Function<String, List<T>> parser = (s) -> | ||
parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList()); | ||
|
||
return new ListSetting<>(key, defaultStringValue, parser, properties); | ||
return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We push the fallback setting down to the concrete setting.
* @return true if the setting and optionally fallback settings is present in the given settings instance, otherwise false | ||
*/ | ||
public boolean exists(final Settings settings, final boolean fallback) { | ||
Setting<?> current = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be easier to read if you pulled the fallback
case out of the loop.
I wonder if instead of exists(setting, true)
we should do settingOrFallbackExists(setting)
. Changing behavior based on method name seems more obvious to me than a boolean parameter. I know you IntelliJ folks get the name of boolean parameters inline but the rest of us don't so we have to either remember or reread the method definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on the naming suggested here. A differently named method is much clearer than an optional extra argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments. Looks good otherwise.
* @return true if the setting and optionally fallback settings is present in the given settings instance, otherwise false | ||
*/ | ||
public boolean exists(final Settings settings, final boolean fallback) { | ||
Setting<?> current = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on the naming suggested here. A differently named method is much clearer than an optional extra argument.
if (fallback == false) { | ||
break; | ||
} | ||
current = current.fallbackSetting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you call current.fallbackSetting.exists(settings, true)
instead of a loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great suggestion. I pushed 7ae63df.
} | ||
|
||
private boolean exists(final Settings settings, final boolean includeFallbackSettings) { | ||
Setting<?> current = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this variable now that you are recurring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine run the java11 tests |
Today when checking settings dependencies, we do not check if fallback settings are present. This means, for example, that if cluster.remote.*.seeds falls back to search.remote.*.seeds, and cluster.remote.*.skip_unavailable and search.remote.*.skip_unavailable depend on cluster.remote.*.seeds, and we have set search.remote.*.seeds and search.remote.*.skip_unavailable, then validation will fail because it is expected that cluster.ermote.*.seeds is set here. This commit addresses this by also checking fallback settings when validating dependencies. To do this, we adjust the settings exist method to also check for fallback settings, a case that it was not handling previously.
* master: (30 commits) Include fallback settings when checking dependencies (elastic#33522) [DOCS] Fixing formatting issues in breaking changes CRUD: Disable wait for refresh tests with delete Test: Fix test name (elastic#33510) HLRC: split ingest request converters (elastic#33435) Logging: Configure the node name when we have it (elastic#32983) HLRC: split xpack request converters (elastic#33444) HLRC: split watcher request converters (elastic#33442) HLRC: add enable and disable user API support (elastic#33481) [DOCS] Fixes formatting error TEST: Ensure merge triggered in _source retention test (elastic#33487) [ML] Add a file structure determination endpoint (elastic#33471) HLRC: ML Forecast Job (elastic#33506) HLRC: split migration request converters (elastic#33436) HLRC: split snapshot request converters (elastic#33439) Make Watcher validation message copy/pasteable Removes redundant test method in SQL tests (elastic#33498) HLRC: ML Post Data (elastic#33443) Pass Directory instead of DirectoryService to Store (elastic#33466) Collapse package structure for metrics aggs (elastic#33463) ...
Today when checking settings dependencies, we do not check if fallback settings are present. This means, for example, that if
cluster.remote.*.seeds
falls back tosearch.remote.*.seeds
, andcluster.remote.*.skip_unavailable
andsearch.remote.*.skip_unavailable
depend oncluster.remote.*.seeds
, and we have setsearch.remote.*.seeds
andsearch.remote.*.skip_unavailable
, then validation will fail because it is expected thatcluster.remote.*.seeds
is set here. This commit addresses this by also checking fallback settings when validating dependencies. To do this, we add a new settings exist method that also checks for fallback settings, a case that was not possible previously.Relates #33413