Allow affix settings to specify dependencies by s1monw · Pull Request #27161 · elastic/elasticsearch

s1monw · 2017-10-29T14:12:13Z

We use affix settings to group settings / values under a certain namespace.
In some cases like login information for instance a setting is only valid if
one or more other settings are present. For instance x.test.user is only valid
if there is an x.test.passwd present and vice versa. This change allows to specify
such a dependency to prevent settings updates that leave settings in an inconsistent
state.

We use affix settings to group settings / values under a certain namespace. In some cases like login information for instance a setting is only valid if one or more other settings are present. For instance `x.test.user` is only valid if there is an `x.test.passwd` present and vice versa. This change allows to specify such a dependency to prevent settings updates that leave settings in an inconsistent state.

s1monw · 2017-10-29T14:12:30Z

/cc @javanna

javanna · 2017-10-31T10:09:13Z

@s1monw I have been trying this out in #27182. Do I understand correctly that a setting which is dependent on another setting must always be provided in the same request as its dependency? That makes sense for user and password, but in CCS one may want to change the skip_if_disconnected setting without touching the seeds, in which case an error is thrown as seeds haven't been specified in the same request, although they were previously set for that remote cluster. Not even sure if this is possible, but we may want to consider this. Thoughts?

You can reproduce with the following unit test:

AbstractScopedSettings service = new ClusterSettings(Settings.builder().put("search.remote.foo.seeds", seed).build(),
                        new HashSet<>(Arrays.asList(RemoteClusterAware.REMOTE_CLUSTERS_SEEDS,
                                RemoteClusterService.REMOTE_CLUSTER_SKIP_IF_DISCONNECTED)));
service.validate(Settings.builder().put("search.remote.foo.skip_if_disconnected", randomBoolean()));

s1monw · 2017-11-01T10:43:36Z

@javanna I applied your feedback thx

s1monw · 2017-11-01T19:31:18Z

@elasticmachine test this please

s1monw · 2017-11-02T09:42:33Z

@javanna can you take another look?

javanna

I tested it out and it works well with the changes from #27182 . I left a few minor comments and a couple of questions. The only minor thing that I wish we could improve, but I am not sure we can, is the error message when you set to null a setting that another one left behind depends on. It may not be clear to users what they have to do to fix that (e.g. set to null the other setting too).

javanna · 2017-11-02T14:30:12Z

...epository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java

-    public static final Setting<String> PROXY_HOST_SETTING = Setting.affixKeySetting(PREFIX, "proxy.host",
-        (key) -> Setting.simpleString(key, Property.NodeScope));
+    public static final Setting<String> PROXY_HOST_SETTING = Setting.affixKeySetting("azure.client.", "proxy.host",
+        (key) -> Setting.simpleString(key, Property.NodeScope), KEY_SETTING, ACCOUNT_SETTING, KEY_SETTING);


I guess KEY_SETTING shouldn't be repeated twice?

javanna · 2017-11-02T14:30:49Z

...epository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java

-    public static final Setting<Integer> PROXY_PORT_SETTING = Setting.affixKeySetting(PREFIX, "proxy.port",
-        (key) -> Setting.intSetting(key, 0, 0, 65535, Setting.Property.NodeScope));
+    public static final Setting<Integer> PROXY_PORT_SETTING = Setting.affixKeySetting("azure.client.", "proxy.port",
+        (key) -> Setting.intSetting(key, 0, 0, 65535, Setting.Property.NodeScope), ACCOUNT_SETTING, KEY_SETTING);


wondering, shouldn't it also depend on proxy_host ?

javanna · 2017-11-02T14:31:44Z

...epository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java

-        (key) -> new Setting<>(key, "direct", s -> Proxy.Type.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope));
+    public static final AffixSetting<Proxy.Type> PROXY_TYPE_SETTING = Setting.affixKeySetting("azure.client.", "proxy.type",
+        (key) -> new Setting<>(key, "direct", s -> Proxy.Type.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope)
+        , ACCOUNT_SETTING, KEY_SETTING);


not too familiar with these settings, does proxy_type depend also on proxy_host and proxy_port?

yeah I am not sure about that

javanna · 2017-11-02T14:34:33Z

core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java

+
+    }
+
+


nit: remove this additional empty line

javanna · 2017-11-02T14:37:55Z

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

+        /**
+         * Returns distinct namespaces for the given settings
+         */
+        public Set<String> getNamespaces(Settings settings) {


nit: match(key) could be replaced with a method reference this::match

javanna · 2017-11-02T14:40:17Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java

                            }
-                            metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(indexSettings));
+                            Settings finalSettings = indexSettings.build();
+                            indexScopedSettings.validate(finalSettings.filter(k -> indexScopedSettings.isPrivateSetting(k) == false), true);


question: private settings are internal only, hence we want to get them out of the validation?

javanna · 2017-11-02T14:43:05Z

core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java

        if (changed) {
+            Settings transientFinalSettings = transientSettings.build();
+            Settings persistentFinalSettings = persistentSettings.build();
+            // both transient and persistent settings must be consistent by itself we can't allow dependencies to be


"must be consistent on their own?"

"we can't allow dependencies between the two"?

what if you have setting A to be transient and then it disappears on a full cluster restart?

sure I agree, I was just suggesting to rephrase the comment :)

javanna · 2017-11-02T14:43:50Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexTemplateService.java


        try {
-            indexScopedSettings.validate(request.settings);
+            indexScopedSettings.validate(request.settings, true); // templates must be consistent with regards to dependecies


dependencies

javanna · 2017-11-02T14:49:17Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java

-                            metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(indexSettings));
+                            Settings finalSettings = indexSettings.build();
+                            indexScopedSettings.validate(finalSettings.filter(k -> indexScopedSettings.isPrivateSetting(k) == false), true);
+                            metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(finalSettings));


I was wondering if this second step wasn't enough, compared to the first validation step that doesn't look at dependencies. Why have the two steps?

one happens before we pass on to a clusterstate update task we should validate as soon as we can. but we have to do it here again

jasontedor

This looks good to me, I left one minor request.

jasontedor · 2017-11-03T00:04:04Z

...ava/org/elasticsearch/action/admin/indices/template/put/TransportPutIndexTemplateAction.java

        final Settings.Builder templateSettingsBuilder = Settings.builder();
        templateSettingsBuilder.put(request.settings()).normalizePrefix(IndexMetaData.INDEX_SETTING_PREFIX);
-        indexScopedSettings.validate(templateSettingsBuilder);
+        indexScopedSettings.validate(templateSettingsBuilder.build(), true); // templates must be consistent with reg. to dependencies


Can you expand reg.? It took me a few seconds to figure out what it meant (regards) and I do not want to have to do it every time I read this comment?

rjernst · 2017-11-08T20:12:28Z

...epository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java


 public final class AzureStorageSettings {
-    // prefix for azure client settings
-    private static final String PREFIX = "azure.client.";


Why remove the internal constant? Seems like this is just asking for a typo in the future in one setting to cause a storm of problems :/

fair enough. I can add it back.

We use affix settings to group settings / values under a certain namespace. In some cases like login information for instance a setting is only valid if one or more other settings are present. For instance `x.test.user` is only valid if there is an `x.test.passwd` present and vice versa. This change allows to specify such a dependency to prevent settings updates that leave settings in an inconsistent state.

* es/master: (24 commits) Reduce synchronization on field data cache add json-processor support for non-map json types (#27335) Properly format IndexGraveyard deletion date as date (#27362) Upgrade AWS SDK Jackson Databind to 2.6.7.1 Stop responding to ping requests before master abdication (#27329) [Test] Fix POI version in packaging tests Allow affix settings to specify dependencies (#27161) Tests: Improve size regex in documentation test (#26879) reword comment Remove unnecessary logger creation for doc values field data [Geo] Decouple geojson parse logic from ShapeBuilders [DOCS] Fixed link to docker content Plugins: Add versionless alias to all security policy codebase properties (#26756) [Test] #27342 Fix SearchRequests#testValidate [DOCS] Move X-Pack-specific Docker content (#27333) Fail queries with scroll that explicitely set request_cache (#27342) [Test] Fix S3BlobStoreContainerTests.testNumberOfMultiparts() Set minimum_master_nodes to all nodes for REST tests (#27344) [Tests] Relax allowed delta in extended_stats aggregation (#27171) Remove S3 output stream (#27280) ...

* 6.x: (27 commits) Reduce synchronization on field data cache [Test] #27342 Fix SearchRequests#testValidate Fail queries with scroll that explicitely set request_cache (#27342) add json-processor support for non-map json types (#27335) Upgrade AWS SDK Jackson Databind to 2.6.7.1 Properly format IndexGraveyard deletion date as date (#27362) Stop responding to ping requests before master abdication (#27329) [Test] Fix POI version in packaging tests Added release notes for 6.0.0 Update 6.0.0-beta1.asciidoc Allow affix settings to specify dependencies (#27161) Tests: Improve size regex in documentation test (#26879) reword comment Ensure external refreshes will also refresh internal searcher to minimize segment creation (#27253) Remove unnecessary logger creation for doc values field data [DOCS] Fixed link to docker content Plugins: Add versionless alias to all security policy codebase properties (#26756) [DOCS] Move X-Pack-specific Docker content (#27333) [Test] Fix S3BlobStoreContainerTests.testNumberOfMultiparts() Set minimum_master_nodes to all nodes for REST tests (#27344) ...

s1monw added :Core/Infra/Settings Settings infrastructure and APIs >enhancement v6.1.0 v7.0.0 labels Oct 29, 2017

s1monw requested a review from jasontedor October 29, 2017 14:12

s1monw added 2 commits October 29, 2017 15:37

remove redud. mod

ddd564c

Merge branch 'master' into settings_dependencies

9417ac6

javanna mentioned this pull request Oct 31, 2017

Cross Cluster Search: make remote clusters optional #27182

Merged

apply feedback from @javanna

278f416

s1monw force-pushed the settings_dependencies branch from c700c5e to 278f416 Compare November 1, 2017 11:03

add missing file

9ef0b0f

Merge branch 'master' into settings_dependencies

9c0d9de

javanna approved these changes Nov 2, 2017

View reviewed changes

s1monw added 3 commits November 2, 2017 17:09

apply feedback

7ea1849

apply feedback

e8e25fb

Merge branch 'master' into settings_dependencies

f3cf5a8

jasontedor approved these changes Nov 3, 2017

View reviewed changes

expand reg. regards

a5d5135

rjernst reviewed Nov 8, 2017

View reviewed changes

s1monw added 2 commits November 13, 2017 09:54

Merge branch 'master' into settings_dependencies

da2878e

bring back prefix key

b73286e

s1monw merged commit 2299c70 into elastic:master Nov 13, 2017

s1monw deleted the settings_dependencies branch November 13, 2017 11:06

Conversation

s1monw commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s1monw commented Oct 29, 2017

Uh oh!

javanna commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s1monw commented Nov 1, 2017

Uh oh!

s1monw commented Nov 1, 2017

Uh oh!

s1monw commented Nov 2, 2017

Uh oh!

javanna 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

s1monw commented Oct 29, 2017 •

edited

Loading

javanna commented Oct 31, 2017 •

edited

Loading