Change format how settings represent lists / array #26723

s1monw · 2017-09-20T12:16:46Z

Today we represent each value of a list setting with it's own dedicated key that ends with the index of the value in the list. Aside of the obvious weirdness this has several issues especially if lists are massive since it causes massive runtime penalties when validating settings. Like a list of 100k words will literally cause a create index call to timeout and in-turn massive slowdown on all subsequent validations runs.

This change moves away from the current internal representation towards a single decoded string representation. A list is encoded into a JSON list internally in a fully backwards compatible way. A list is then a single key value pair in settings and all prefix based settings are internally converted to the new representation in the settings builder. This change also forbids to add a settings that ends with a .0 which was internally used to detect a list setting. Once this has been rolled out for an entire major version all the internal .0 handling can be removed since all settings will be converted.

Today we represent each value of a list setting with it's own dedicated key that ends with the index of the value in the list. Aside of the obvious weirdness this has several issues especially if lists are massive since it causes massive runtime penatlies when validating settings. Like a list of 100k words will literally cause a create index call to timeout and in-turn massive slowdown on all subsequent validations runs. This change moves away from the current internal representation towards a single decoded string representation. A list is encoded into a JSON list internally in a fully backwards compatible way. A list is then a single key value pair in settings and all prefix based settings are internally converted to the new representation in the settings builder. This change also forbids to add a settings that ends with a `.0` which was internally used to detect a list setting. Once this has been rolled out for an entire major version all the internal `.0` handling can be removed since all settings will be converted.

nik9000

I looked at it and thought about it some. I've left what I thought. I'll come back and have another look soon.

nik9000 · 2017-09-21T18:50:46Z

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

    }

+    private static boolean isInternalList(String value) {
+        return value != null
+            && value.length() >= 4


I think this should be 2. 4 is the minimum number of bytes but length is in chars.

nik9000 · 2017-09-21T18:55:46Z

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

            }
        }
        return builder;
    }

+    private static List<String>  maybeGetList( String value) throws IOException {
+        if (value != null && isInternalList(value)) { // we try to write it as a list if it is a list


I don't think "write" is correct here.

nik9000 · 2017-09-21T19:11:36Z

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

@@ -739,6 +745,14 @@ public Builder put(Object... settings) {
         * @return The builder
         */
        public Builder put(String key, String value) {
+            if (key.endsWith(".0")) {


This makes me uncomfortable.... I see that we used to do it as well. Can we get away with not doing it anymore now that we have putArray?

nik9000 · 2017-09-21T19:15:49Z

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

@@ -407,21 +407,21 @@ public void testDiff() throws IOException {
        ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(fooBar, fooBarBaz, foorBarQuux,
            someGroup, someAffix)));
        Settings diff = settings.diff(Settings.builder().put("foo.bar", 5).build(), Settings.EMPTY);
-        assertEquals(4, diff.size()); // 4 since foo.bar.quux has 3 values essentially
+        assertEquals(2, diff.size()); // 4 since foo.bar.quux has 3 values essentially


No need for the comment anymore.

nik9000 · 2017-09-21T19:16:17Z

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

        assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(17));
        assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"d", "e", "f"});

        diff = settings.diff(
            Settings.builder().put("some.group.foo", 5).build(),
            Settings.builder().put("some.group.foobar", 17, "some.group.foo", 25).build());
-        assertEquals(6, diff.size()); // 6 since foo.bar.quux has 3 values essentially
+        assertEquals(4, diff.size()); // 6 since foo.bar.quux has 3 values essentially


Same. These comments were helpful but are not incorrect.

nik9000 · 2017-09-21T19:16:25Z

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

        assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1));
        assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"a", "b", "c"});

        diff = settings.diff(
                Settings.builder().put("foo.bar", 5).build(),
                Settings.builder().put("foo.bar.baz", 17).putArray("foo.bar.quux", "d", "e", "f").build());
-        assertEquals(4, diff.size()); // 4 since foo.bar.quux has 3 values essentially
+        assertEquals(2, diff.size()); // 4 since foo.bar.quux has 3 values essentially


nik9000 · 2017-09-21T19:16:29Z

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

@@ -432,7 +432,7 @@ public void testDiff() throws IOException {
            Settings.builder().put("some.prefix.foo.somekey", 5).build(),
            Settings.builder().put("some.prefix.foobar.somekey", 17,
                "some.prefix.foo.somekey", 18).build());
-        assertEquals(6, diff.size()); // 6 since foo.bar.quux has 3 values essentially
+        assertEquals(4, diff.size()); // 6 since foo.bar.quux has 3 values essentially


rjernst · 2017-09-26T18:37:07Z

When discussing this many weeks ago, I suggested having the value of the internal map be a discriminated union. This could either simply be Object with the discrimination done by instanceof checks, or a simple POJO with a boolean+Object. I'm apprehensive about this PR because it means every iteration of a list setting requires creation of new strings (by parsing the "list" string). Why are these special marker strings to denote whether the value is a list any better than actually having a List and using instanceof? In either case, we have to ensure access to the internal map is removed, but the latter is much more transparent and easier to not mess up.

s1monw · 2017-09-27T09:12:33Z

I am closing this one since the encapsulation of Settings is so broken today I first have to clean up all the cruft around it to make it work. I think ultimately we can just have a String,Object map internally but before that we have to get rid of things like getAsMap on settings.

Today we represent each value of a list setting with it's own dedicated key that ends with the index of the value in the list. Aside of the obvious weirdness this has several issues especially if lists are massive since it causes massive runtime penalties when validating settings. Like a list of 100k words will literally cause a create index call to timeout and in-turn massive slowdown on all subsequent validations runs. With this change we use a simple string list to represent the list. This change also forbids to add a settings that ends with a .0 which was internally used to detect a list setting. Once this has been rolled out for an entire major version all the internal .0 handling can be removed since all settings will be converted. Relates to elastic#26723

Today we represent each value of a list setting with it's own dedicated key that ends with the index of the value in the list. Aside of the obvious weirdness this has several issues especially if lists are massive since it causes massive runtime penalties when validating settings. Like a list of 100k words will literally cause a create index call to timeout and in-turn massive slowdown on all subsequent validations runs. With this change we use a simple string list to represent the list. This change also forbids to add a settings that ends with a .0 which was internally used to detect a list setting. Once this has been rolled out for an entire major version all the internal .0 handling can be removed since all settings will be converted. Relates to #26723

s1monw added :Core/Infra/Settings Settings infrastructure and APIs >bug v7.0.0 labels Sep 20, 2017

s1monw requested review from rjernst and jasontedor September 20, 2017 12:16

s1monw added 5 commits September 20, 2017 14:43

add wire level BWC

21e5e66

fix test name

304bb4b

fix tests

d4160df

use intenral markers for list settings

b7f7ccd

Merge branch 'master' into fix_list_settings

f309337

s1monw changed the title ~~Chance format how settings represent lists / array~~ Change format how settings represent lists / array Sep 20, 2017

s1monw added 3 commits September 20, 2017 16:32

remove final modifyer

f856b1a

fix broken ec2 setting

27d3fc3

fix broken ec2 setting

5e23b29

nik9000 reviewed Sep 21, 2017

View reviewed changes

s1monw closed this Sep 27, 2017

s1monw deleted the fix_list_settings branch October 4, 2017 07:24

s1monw mentioned this pull request Oct 4, 2017

Represent lists as actual lists inside Settings #26878

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change format how settings represent lists / array #26723

Change format how settings represent lists / array #26723

Uh oh!

s1monw commented Sep 20, 2017

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 Sep 21, 2017

Uh oh!

nik9000 Sep 21, 2017

Uh oh!

nik9000 Sep 21, 2017

Uh oh!

nik9000 Sep 21, 2017

Uh oh!

nik9000 Sep 21, 2017

Uh oh!

nik9000 Sep 21, 2017

Uh oh!

nik9000 Sep 21, 2017

Uh oh!

rjernst commented Sep 26, 2017

Uh oh!

s1monw commented Sep 27, 2017

Uh oh!

Uh oh!

Change format how settings represent lists / array #26723

Change format how settings represent lists / array #26723

Uh oh!

Conversation

s1monw commented Sep 20, 2017

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

nik9000 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

nik9000 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

nik9000 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

nik9000 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

nik9000 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

nik9000 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

rjernst commented Sep 26, 2017

Uh oh!

s1monw commented Sep 27, 2017

Uh oh!

Uh oh!