Separate out validation of groups of settings #34184

cbismuth · 2018-10-01T13:30:50Z

This pull request allows a setting to be validated against target settings to ensure runtime dependencies are met (e.g. disk watermarks low, high and flood_stage).

This pull request also includes a fix to not update settings when no value has changed.

elasticmachine · 2018-10-02T10:53:56Z

Pinging @elastic/es-core-infra

DaveCTurner

This needs some tests adding. The one I suggested would be valuable, but I think it also needs something that focusses on this change specifically, so that this remains covered even if the disk watermark feature were changed and that test were lost.

DaveCTurner · 2018-10-04T10:37:25Z

@elasticmachine test this please

cbismuth · 2018-10-04T10:47:15Z

Sure, I'll report the test you've suggested and add a new one on this change in particular, thank you @DaveCTurner.

DaveCTurner · 2018-10-04T11:04:45Z

Note that the build that failed looks like it failed for good reason. Could you move this failure into a unit test and then fix it?

cbismuth · 2018-10-04T11:14:35Z

Yes 👍

cbismuth · 2018-10-04T14:05:04Z

Note that the build that failed looks like it failed for good reason. Could you move this failure into a unit test and then fix it?

Issue found and fixed in f77d1e7, thanks @DaveCTurner. I'm now focusing on implementing relevant test cases.

DaveCTurner · 2018-10-04T14:49:06Z

Responding to #28309 (comment) here since this is the right place to discuss this PR.

The single test present in this PR (the current head that I'm looking at is f77d1e7) does not support the change to the production code, in the sense that it passes on today's master as well. To be worthwhile it needs to be a test that fails without the change in place.

The test failure that you found and fixed should also be captured in a test.

Additionally I think we should have a test that focusses on this change to the way that Settings work which is independent of the disk watermark feature.

cbismuth · 2018-10-04T15:07:31Z

I agree with you @DaveCTurner, tests are under development.

Let me please reword: in my previous comment, I meant I've modified the test you suggested and I wanted to know whether or not you agree with the changes I've made in your test. Do you see what I mean?

DaveCTurner · 2018-10-04T15:19:07Z

I wanted to know whether or not you agree with the changes I've made in your test. Do you see what I mean?

I think so, and my feedback was that this test passes on master so doesn't demonstrate the bug that we're chasing, which means it's not the test we need for this change. I suppose it's a reasonable test in the sense that it's expected to pass, but I also think there are already test cases that state the same thing more generally so it's not really necessary.

If there is more work in progress then it's probably best to ping when the whole PR is ready to review - it'll be easier to see the whole picture.

cbismuth · 2018-10-04T15:28:44Z

I understand, thank you.

…parently_valid_sequence_of_updates

cbismuth · 2018-10-05T09:06:51Z

@DaveCTurner PR is complete and ready for review.

cbismuth · 2018-10-05T10:54:00Z

I've found another bug in the AbstractScopedSettings#updateSettings API: the return value was true even when no value has changed.

I've added commit 68044ba to this PR to fix it and I've added a test case to cover this change.

PR is ready for review, I've nothing more to add, thank you.

DaveCTurner

Thanks @cbismuth, this is a tricky area and you've done some good work here. I've left a few comments and suggested an alternative approach that I think would be better, although I haven't tried it so you're welcome to describe why it doesn't work :)

DaveCTurner · 2018-10-06T15:28:33Z

...r/src/test/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettingsTests.java

+        final ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
+        new DiskThresholdSettings(settings, clusterSettings); // this has the effect of registering the settings updater
+
+        settings = clusterSettings.applySettings(Settings.builder()


I see what's happened here, and I must apologise. The unit test I suggested was looking at applySettings but really I should have used updateSettings since this is where the update is calculated. My bad.

However, this test still passes on master without any changes so I don't think it should be added here. Could you make an appropriate test based around updateSettings instead, and check that it fails on master and passes on this branch?

No worries at all. Sure, I'll update the test and run it on this branch and on master.

DaveCTurner · 2018-10-06T15:33:03Z

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

+            updates,
+            "transient"
+        );
+        assertFalse(updated);


I think it's ok to return true here. Claiming to have updated a setting when really it hasn't changed in value is a right-side failure: it means we call reroute() to reallocate any shards affected by the new settings, but this does nothing if the setting change was a no-op. On the other hand failing to call reroute() when a setting does change is very bad, so changing the behaviour here needs a lot more scrutiny.

I understand. I'll restore the previous behavior to not break the current data flow and leave this responsibility to downstream components, it's safer.

I wasn't very at ease with this change, thank you for catching it. I'll dig deeper into it with a debugger just for my own curiosity.

DaveCTurner · 2018-10-06T15:36:32Z

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

+        );
+        assertTrue(updated);
+        assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20"));
+        assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20"));


We can write this with fewer lines and fewer mutable variables by putting each of these steps into its own block like this:

{ final Settings.Builder updates = Settings.builder(); assertTrue(service.updateSettings(Settings.builder().put(SETTING_FOO_LOW.getKey(), 20).build(), target, updates, "transient")); assertThat(target.get(SETTING_FOO_LOW.getKey()), equalTo("20")); assertThat(updates.get(SETTING_FOO_LOW.getKey()), equalTo("20")); }

The line-width limit is 140 characters, and this fits into that very neatly. It's something of a matter of taste, but we seem to prefer denser code in this sort of situation.

That's fine for me. The more immutable code is, the more confident I feel.

DaveCTurner · 2018-10-06T15:38:39Z

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

+            Settings.builder(),
+            "transient"
+        ));
+        assertThat(exception.getMessage(), equalTo("[high]=10 is lower than [low]=20"));


Could we also have a case that shows that if low is set below 10 then high can also be set below 10, to verify that we're not validating against both the default and the actual values?

Sure, I'll add these test cases.

DaveCTurner · 2018-10-06T15:39:46Z

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.sameInstance;

 public class ScopedSettingsTests extends ESTestCase {

+    private static class FooLowSettingValidator implements Setting.Validator<Integer> {


I think these settings can reasonably be moved right next to testUpdateOfValidationDependantSettings since that's the only place they're used.

Yes, that would be better, I'll move them.

DaveCTurner · 2018-10-06T15:48:56Z

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

-                changed = true;
+                changed = hasChanged(toApply, target);
+                if (changed) {
+                    validate(toApply, target.build());


I think this isn't quite the right approach. As I understand it this change means we're validating the whole settings update here, but by my reading we already validate that later on in the process:

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

Lines 94 to 95 in 4dc3ada

clusterSettings.validate(transientFinalSettings, true);

clusterSettings.validate(persistentFinalSettings, true);

I think it'd be better to try and do less validation here on settings that have dependent settings. Perhaps the Setting.Validator interface should have two validate() methods, one for this stage of validation (good for settings with no dependencies, i.e., most of them) and one for the full validation including dependencies.

Yes, I see what you mean. I'll try to move the full validation including dependencies in a new API in Setting.Validator. I also think it's a better location for this.

DaveCTurner · 2018-10-06T15:51:23Z

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

+
+    private boolean hasChanged(Settings toApply, Settings.Builder target) {
+        boolean changed = toApply.keySet().stream().anyMatch(k -> k.endsWith("*"));
+        if (!changed) {


The house style is to avoid using the ! unary operator and to prefer saying changed == false here, because ! is small and easy to miss in a long expression, and == false is larger and therefore more visible. I think this code will go away due to other comments, but thought I'd raise it anyway for your info.

Thanks for sharing this guideline with me, it's indeed more readable without the unary operator.

cbismuth · 2018-10-08T09:55:05Z

Thank you @DaveCTurner for the kind words and insightful review. I'll rework my changes and let you know when it's ready.

…parently_valid_sequence_of_updates

s1monw

LGTM I left a suggestion regarding the interface

s1monw · 2018-11-15T20:27:26Z

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

         *
         * @param value    the value of this setting
         * @param settings a map from the settings specified by {@link #settings()}} to their values
         */
-        void validate(T value, Map<Setting<T>, T> settings);
+        default void validate(T value, Map<Setting<T>, T> settings) {


does it make sense to delegate to validate(T value) here?

from an interface perspective I think we might be able to only have validate(T value, Optional<Map<Setting<T>, T>> settings) and instead check the optional if needed?

Thanks @s1monw.

I'd prefer to keep these two API as most clients will only be interested in validating a setting in isolation (i.e. not against dependencies).
Besides, we do a fail-fast validation here where we don't have resolved setting dependencies yet. Therefore, if we want to keep this fail-fast shortcut, any client will have to implement an if optional present/absent block even if he's interested in validating a setting against its dependencies.

What do you think about these two points?

I think I prefer the two methods to the optional parameter too. In practice there are very few dependencies between settings, so the one-argument lambda is less noisy in almost all cases. Settings with dependencies can't use a lambda anyway because they have to override the settings() function too.

I also think there's no need for the dependent validation to delegate to the other one, because we call both during settings validation. However I think this should be mentioned in the Javadocs as implementors shouldn't need to go to the implementation to check this (and I know I'd forget and have to check). Also the interface-level comment needs updating as it's no longer wholly accurate.

Hi @DaveCTurner, here is a additional commit 7c33bfe to improve the Validator class documentation, thank you.

I don't have strong feelings but if I see two methods I'd only implement one and don't know what I need to do with the other. The optional makes me think about the opportunity to validate against others. I don't know what you mean by clients?

I meant developers who will implement this interface.

We tried to make javadoc more explicit, but I see what you mean: the API doesn't enforce the developer to ask himself whether or not he would like to implement dependencies validation. But as it's an edge case, I still hope the developer would read the javadoc or jump to the interface definition if he needs to do so.

Another alternative would be to remove the validation shortcut and keep only the validate(T value, Map<Setting<T>, T> settings) interface API, I think that would make sense.

We would remove this validation shortcut here and keep only this validation call here (and remove this line).

What do you think about it?

DaveCTurner

I left a comment about Javadocs using the wrong button -> https://github.com/elastic/elasticsearch/pull/34184/files#r234140602

…parently_valid_sequence_of_updates

cbismuth · 2018-11-23T09:57:29Z

Hi @s1monw and @DaveCTurner, I'm sure everyone is quite busy, so here is a quick follow up below to ease PR validation and merge.

This PR separates out validation of groups of settings,
A fail-fast validation is done early in the code to check each updated setting without resolving its dependencies,
A full validation is done later to check dependencies of each updated setting,
We've introduced a new API to validate dependencies of a setting and it is a no-op default method in the Validator<T> interface,
@s1monw would prefer to have a single API to implement in the Validator<T> interface without the no-op default interface method to validate dependencies of a setting.

We have three options:

Leave things as they are now and rely on the updated Java documentation to tell developers there's a no-op method to override to validate dependencies of a setting.
Refactor the Validator<T> interface to have a single validate(T value, Optional<Map<Setting<T>, T>> settings) API and delegate to developers the task to check whether or not the optional parameter is present or absent (it will always be absent when the early fail-fast first check is done).
Refactor the Validator<T> interface to have a single validate(T value, Map<Setting<T>, T> settings) API and remove the early fail-fast first check so that the map parameter will always be present (not empty when a setting has dependencies or empty when a setting has no dependency).

I would prefer option 3, and you?

…parently_valid_sequence_of_updates

cbismuth · 2018-11-25T16:04:13Z

We have a green build 👍

cbismuth · 2018-11-30T10:30:35Z

Hi, I know it's Wednesday and there're probably many things to close before the weekend.

I hope next week we will be able state whether or not this PR can be merged as it is (with latest documentation fix included) or should we go for a new iteration to simplify the Validator<T> interface.

The more I look at it, the more I think this version is a good one and we should merge it as it is.

cbismuth · 2018-12-10T14:22:54Z

Hi @DaveCTurner, what do you think we should do on this PR? Thanks.

…parently_valid_sequence_of_updates

DaveCTurner · 2018-12-12T12:51:45Z

Hi @cbismuth, just to let you know that this PR is still in my queue. Bear with me, I have a few too many things in flight right now.

cbismuth · 2018-12-12T18:07:36Z

No worries, thank you @DaveCTurner, I'll be available when needed.

…parently_valid_sequence_of_updates

DaveCTurner

I made some minor changes to the Javadocs in 02a62fb and merged a recent master in, and now this LGTM. Thanks for your patience @cbismuth.

cbismuth · 2019-01-07T13:19:35Z

That's great! Thanks a lot @DaveCTurner for finely reviewing the code and no worries at all for the time it took.

Today, a setting can declare that its validity depends on the values of other related settings. However, the validity of a setting is not always checked against the correct values of its dependent settings because those settings' correct values may not be available when the validator runs. This commit separates the validation of a settings updates into two phases, with separate methods on the `Setting.Validator` interface. In the first phase the setting's validity is checked in isolation, and in the second phase it is checked again against the values of its related settings. Most settings only use the first phase, and only the few settings with dependencies make use of the second phase.

…piration * elastic/master: Removing unused methods in Numbers (elastic#37186) Fix setting by time unit (elastic#37192) [DOCS] Cleans up xpackml attributes ML: fix delayed data annotations on secured cluster (elastic#37193) Update version in SearchRequest and related test [DOCS] Adds overview and API ref for cluster voting configurations (elastic#36954) ML: changing JobResultsProvider.getForecastRequestStats to support > 1 index (elastic#37157) Separate out validation of groups of settings (elastic#34184)

Validate current setting against all target settings (#28309)

8852b05

cbismuth mentioned this pull request Oct 1, 2018

Disk watermark validation rejects apparently valid sequence of updates #28309

Closed

colings86 added the :Core/Infra/Settings Settings infrastructure and APIs label Oct 2, 2018

javanna added the >enhancement label Oct 2, 2018

Fix misspelled comment (#28309)

5a84c18

DaveCTurner requested changes Oct 4, 2018

View reviewed changes

cbismuth added 2 commits October 4, 2018 14:46

Test sequence of updates (#28309)

71cfe1a

Add setting to apply to validation collection (#28309)

f77d1e7

cbismuth added 4 commits October 4, 2018 22:53

Test update of validation dependant settings (#28309)

b0dd2de

Fix comment (#28309)

177f560

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

ad0212e

…parently_valid_sequence_of_updates

Improve variable naming to ease readability (#28309)

94ac1d1

Don't update settings when no value has changed (#28309)

68044ba

DaveCTurner requested changes Oct 6, 2018

View reviewed changes

DaveCTurner self-assigned this Oct 6, 2018

cbismuth added 2 commits October 8, 2018 12:06

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

395c398

…parently_valid_sequence_of_updates

Move validation API with other ones (#28309)

8e138d9

cbismuth added 4 commits November 5, 2018 11:38

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

64aeb4c

…parently_valid_sequence_of_updates

Remove unnecessary parentheses around lambdas (#28309)

bfcd828

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

453a03a

…parently_valid_sequence_of_updates

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

e83e210

…parently_valid_sequence_of_updates

s1monw approved these changes Nov 15, 2018

View reviewed changes

DaveCTurner requested changes Nov 16, 2018

View reviewed changes

cbismuth added 4 commits November 16, 2018 12:00

Fix Java documentation of the Validator class (#elasticsearch-28309)

7c33bfe

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

17b3ab2

…parently_valid_sequence_of_updates

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

adf5afb

…parently_valid_sequence_of_updates

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

e277b81

…parently_valid_sequence_of_updates

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

da718f1

…parently_valid_sequence_of_updates

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

f76692f

…parently_valid_sequence_of_updates

DaveCTurner added 2 commits January 7, 2019 11:14

Merge branch 'master' into 28309_disk_watermark_validation_rejects_ap…

de93e7c

…parently_valid_sequence_of_updates

Minor Javadoc rewording

02a62fb

DaveCTurner approved these changes Jan 7, 2019

View reviewed changes

DaveCTurner merged commit 9602d79 into elastic:master Jan 7, 2019

DaveCTurner added v7.0.0 v6.7.0 labels Jan 9, 2019

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

cbismuth deleted the 28309_disk_watermark_validation_rejects_apparently_valid_sequence_of_updates branch April 3, 2019 21:12

	clusterSettings.validate(transientFinalSettings, true);
	clusterSettings.validate(persistentFinalSettings, true);

Separate out validation of groups of settings #34184

Separate out validation of groups of settings #34184

Conversation

cbismuth commented Oct 1, 2018 • edited Loading

elasticmachine commented Oct 2, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 4, 2018

cbismuth commented Oct 4, 2018

DaveCTurner commented Oct 4, 2018

cbismuth commented Oct 4, 2018

cbismuth commented Oct 4, 2018

DaveCTurner commented Oct 4, 2018

cbismuth commented Oct 4, 2018

DaveCTurner commented Oct 4, 2018

cbismuth commented Oct 4, 2018

cbismuth commented Oct 5, 2018

cbismuth commented Oct 5, 2018 • edited Loading

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbismuth commented Oct 8, 2018

s1monw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbismuth Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

cbismuth Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

cbismuth commented Nov 23, 2018 • edited Loading

cbismuth commented Nov 25, 2018

cbismuth commented Nov 30, 2018 • edited Loading

cbismuth commented Dec 10, 2018

DaveCTurner commented Dec 12, 2018

cbismuth commented Dec 12, 2018 • edited Loading

DaveCTurner left a comment

Choose a reason for hiding this comment

cbismuth commented Jan 7, 2019

cbismuth commented Oct 1, 2018 •

edited

Loading

cbismuth commented Oct 5, 2018 •

edited

Loading

cbismuth Nov 19, 2018 •

edited

Loading

cbismuth Nov 19, 2018 •

edited

Loading

cbismuth commented Nov 23, 2018 •

edited

Loading

cbismuth commented Nov 30, 2018 •

edited

Loading

cbismuth commented Dec 12, 2018 •

edited

Loading