Enable cross-setting validation by jasontedor · Pull Request #25560 · elastic/elasticsearch

jasontedor · 2017-07-05T19:46:17Z

This commit introduces a framework for settings validation and enables cross-setting validation.

This commit enables multiple settings validation during dynamic updates and applies it to the disk threshold watermark settings.

jasontedor · 2017-07-05T19:46:31Z

@s1monw I'm opening this for discussion.

jasontedor · 2017-07-05T19:47:44Z

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

+
+        void validate(T value, Map<Setting<T>, T> settings);
+
+        default Iterator<Setting<T>> settings() {


Right now this carries the assumption that the settings that we validate against have the same type as the setting itself. I think this is okay for now, we can revisit later only if we need to?

s1monw · 2017-07-05T19:51:16Z

thanks @jasontedor this is a big step. Yet, I think we need to move this kind of validation to the settings itself. We need to apply the validation to the entire Settings instance after the update or before the node starts. Otherwise we might fail way too late. I also wonder if the order you pass the settings matters in your solution but I haven't verified that so I might be wrong?

jasontedor · 2017-07-05T21:57:37Z

@s1monw I pushed 55a3d3b; let me know what you think?

bleskes · 2017-07-05T22:13:26Z

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

+                } else {
+                    map = Collections.emptyMap();
+                }
+                validator.validate(parsed, map);


just a drive by question - why not pass the entire settings object and let the (exceptional) validate logic look at whatever they want?

@bleskes The reason for this is because then the validator need to pull out the settings that they want, which means parsing the settings, which means exercising the validation logic which leads to stack overflow unless we expose the skip validation override as part of the public API of Setting which I think is a bad idea.

I hear you. There is still the alternative of let people use Setting#get for most things as I think this is just an issue with circular validation (i.e., two settings that validate each other). Where there is a circular validation, I think we typically can solve it by validating only on one of the keys (as always validate all keys). If things really get complicated people can always use Setting#getRaw and skip validation (at the price of doing parsing). Just an idea to simplify this which I wanted to hear your thoughts on. Totally up to you.

I think that this would not quite work. Suppose that setting A is not dynamic, and setting B is. If we have validation on A between A and B but not validation on B between A and B, then an update for B will not exercise the validation leaving broken settings.

One can say: okay, only have the validation on the dynamic setting. Then I think it's odd, it relies on us remembering this fact, and it's odd to not have symmetric validation anyway.

I prefer it as is?

I prefer it as is?

As said - totally fine. Thanks for picking this up.

* master: Refactor PathTrie and RestController to use a single trie for all methods (elastic#25459) Switch indices read-only if a node runs out of disk space (elastic#25541)

jasontedor · 2017-07-06T02:37:53Z

@s1monw This PR was getting big (due to all the tests) so I broke it into two PRs by factoring out the changes that inspired this into a separate commit (jasontedor/elasticsearch@ e346e1a5a04aab45653e27b217f523efa7ece804) which I will open as a follow-up PR to this one.

jasontedor · 2017-07-06T19:39:20Z

retest this please

s1monw

so I like the imp, I have one question. Lets say we have 2 settings A and B that are depending on each other. Now a > b all the time. Now we are updating the settings in one go setting a=5,b=4 but the current settings are a=20,b=10 if you use the updater it will still work I think since we always pass consistent views to the updater, right?

s1monw · 2017-07-07T07:54:15Z

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

        try {
-            return parser.apply(value);
+            T parsed = parser.apply(value);
+            if (validate) {


should we check here if we have a validator at all and only check if we do?

We always have a validator as we initialize with a trivial validator in cases one is not provided when the Setting is defined.

s1monw · 2017-07-07T08:07:11Z

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

+                    map = new HashMap<>();
+                    while (it.hasNext()) {
+                        final Setting<T> setting = it.next();
+                        map.put(setting, setting.get(settings, false)); // we have to disable validation or we will stack overflow


another option would be to use #get(Settings, Validator) so we can just have a simple null check instead of if (validate) and we make it nullable instead of a no-op?

I tend to favor it as-is. The reason for this is because have the field validator in the Setting instance, and if we pass a validator into this method we have validator from the Setting instance and the passed-in validator and I think this can be confusing. Now one way around this is to make get(Settings, Validator<T>) static so that it can not see the instance Validator<T> but then this gets ugly because of generics, and because it invokes other instance methods.

jasontedor · 2017-07-07T13:58:40Z

Lets say we have 2 settings A and B that are depending on each other. Now a > b all the time. Now we are updating the settings in one go setting a=5,b=4 but the current settings are a=20,b=10 if you use the updater it will still work I think since we always pass consistent views to the updater, right?

That's correct. That happens because we push the new Settings instance down when doing these updates and that's where the validation happens against.

Enable multiple settings validation

54bae69

This commit enables multiple settings validation during dynamic updates and applies it to the disk threshold watermark settings.

jasontedor added :Core/Infra/Settings Settings infrastructure and APIs >enhancement review v6.0.0 labels Jul 5, 2017

jasontedor requested a review from s1monw July 5, 2017 19:46

jasontedor mentioned this pull request Jul 5, 2017

Switch indices read-only if a node runs out of disk space #25541

Merged

jasontedor commented Jul 5, 2017

View reviewed changes

jasontedor added 2 commits July 5, 2017 17:56

Validate settings

55a3d3b

Add comment

1c3ba96

jasontedor added 5 commits July 5, 2017 17:58

Fix parameter name

a4953dc

Revert some changes

54d61e1

Another test

0a2d755

Remove some duplicate code

aa7c150

Rearrange

48493c0

bleskes reviewed Jul 5, 2017

View reviewed changes

jasontedor added 9 commits July 5, 2017 18:14

Fix slow log tests

0ba6e61

Fix setting tests

921aaac

Fix condition

882a2dd

Fix tests

01f0dbf

Merge branch 'master' into multiple-settings-validation

4562b30

* master: Refactor PathTrie and RestController to use a single trie for all methods (elastic#25459) Switch indices read-only if a node runs out of disk space (elastic#25541)

Test flood settings too

9eb8402

Add another test

e6fe3c9

Revert disk changes for another PR

d14d194

Add validation test

0e6ff0a

jasontedor changed the title ~~Enable multiple settings validation~~ Enable cross-setting validation Jul 6, 2017

jasontedor added 2 commits July 5, 2017 22:44

Javadocs

6dc8512

Assert not mutation on invalid

91f7f56

s1monw approved these changes Jul 7, 2017

View reviewed changes

s1monw reviewed Jul 7, 2017

View reviewed changes

jasontedor merged commit 5762bce into elastic:master Jul 7, 2017

jasontedor mentioned this pull request Jul 7, 2017

Add disk threshold settings validation #25600

Merged

jasontedor deleted the multiple-settings-validation branch July 10, 2017 14:44

colings86 added v6.0.0-beta1 and removed v6.0.0 labels Jul 31, 2017

dnhatn mentioned this pull request Aug 29, 2018

Integrates soft-deletes into Elasticsearch #33222

Merged

jasontedor mentioned this pull request Sep 28, 2019

Allow setting validation against arbitrary types #47264

Merged


		void validate(T value, Map<Setting<T>, T> settings);

		default Iterator<Setting<T>> settings() {

Conversation

jasontedor commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jasontedor commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Jul 6, 2017

Uh oh!

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

jasontedor commented Jul 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jasontedor commented Jul 5, 2017 •

edited

Loading

s1monw commented Jul 5, 2017 •

edited

Loading

jasontedor Jul 5, 2017 •

edited

Loading

jasontedor Jul 6, 2017 •

edited

Loading

jasontedor commented Jul 6, 2017 •

edited

Loading