Include fallback settings when checking dependencies by jasontedor · Pull Request #33522 · elastic/elasticsearch

jasontedor · 2018-09-07T17:04:07Z

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.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

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.

elasticmachine · 2018-09-07T17:04:08Z

Pinging @elastic/es-core-infra

jasontedor · 2018-09-07T17:07:27Z

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

            throw new IllegalArgumentException(msg);
        } else  {
-            Set<String> settingsDependencies = setting.getSettingsDependencies(key);
+            Set<Setting<?>> settingsDependencies = setting.getSettingsDependencies(key);


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.

jasontedor · 2018-09-07T17:07:43Z

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

-                        throw new IllegalArgumentException("Missing required setting ["
-                            + requiredSetting + "] for setting [" + setting.getKey() + "]");
+                for (final Setting<?> settingDependency : settingsDependencies) {
+                    if (settingDependency.exists(settings) == false) {


Here we change from matching the key to using exists, so that we can use fallback settings if needed.

jasontedor · 2018-09-07T17:08:14Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

-    public boolean exists(Settings settings) {
-        return settings.keySet().contains(getKey());
+    public boolean exists(final Settings settings) {
+        Setting<?> current = this;


This method was not checking fallback settings, so we could get false here if the fallback setting was set but not the actual setting.

jasontedor · 2018-09-07T17:09:00Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

        }
    }

-    private static class ListSetting<T> extends Setting<List<T>> {


I moved this code so that it's closer to where ListSettings are used.

jasontedor · 2018-09-07T17:11:21Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

+
+        private ListSetting(
+                final String key,
+                final @Nullable Setting<List<T>> fallbackSetting,


Fallback settings were not being pushed down here, so ListSettings did not know until this change that they could have fallback settings.

jasontedor · 2018-09-07T17:11:43Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

                parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());

-        return new ListSetting<>(key, defaultStringValue, parser, properties);
+        return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, properties);


We push the fallback setting down to the concrete setting.

nik9000 · 2018-09-07T17:45:28Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

+     * @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;


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.

I agree on the naming suggested here. A differently named method is much clearer than an optional extra argument.

@nik9000 I pushed eb9be45. Let me know what you think?

rjernst

A couple comments. Looks good otherwise.

rjernst · 2018-09-07T17:54:04Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

+     * @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;


I agree on the naming suggested here. A differently named method is much clearer than an optional extra argument.

rjernst · 2018-09-07T17:55:32Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

+            if (fallback == false) {
+                break;
+            }
+            current = current.fallbackSetting;


Can't you call current.fallbackSetting.exists(settings, true) instead of a loop?

That is a great suggestion. I pushed 7ae63df.

nik9000 · 2018-09-07T18:10:34Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

+    }
+
+    private boolean exists(final Settings settings, final boolean includeFallbackSettings) {
+        Setting<?> current = this;


I don't think you need this variable now that you are recurring.

nik9000

LGTM

jasontedor · 2018-09-07T18:45:32Z

@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.

jasontedor · 2018-09-08T02:40:47Z

Thanks @nik9000 and @rjernst for a quite helpful review.

* 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) ...

jasontedor added review :Core/Infra/Settings Settings infrastructure and APIs v7.0.0 v6.5.0 labels Sep 7, 2018

jasontedor requested a review from s1monw September 7, 2018 17:04

jasontedor commented Sep 7, 2018

View reviewed changes

jasontedor added 2 commits September 7, 2018 13:34

Fix test

cafc63c

Javadocs

3fb9f12

nik9000 reviewed Sep 7, 2018

View reviewed changes

Iteration

eb9be45

rjernst approved these changes Sep 7, 2018

View reviewed changes

Use recursion

7ae63df

nik9000 reviewed Sep 7, 2018

View reviewed changes

jasontedor added 2 commits September 7, 2018 14:17

Simplify

45b63d3

Fix test

4c07555

nik9000 approved these changes Sep 7, 2018

View reviewed changes

jasontedor added 2 commits September 7, 2018 16:06

Fix more tests

d469474

Fix another test

fe724e2

jasontedor merged commit 9a404f3 into elastic:master Sep 8, 2018

jasontedor deleted the fallback-settings-dependencies branch September 8, 2018 02:40

colings86 added the >non-issue label Oct 25, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

jasontedor commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Sep 7, 2018

Uh oh!

jasontedor commented Sep 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jasontedor commented Sep 7, 2018 •

edited

Loading