Represent lists as actual lists inside Settings #26878
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
s1monw
added
:Core/Infra/Settings
rjernst
approved these changes
Oct 4, 2017
| * @param defaultArray The default array to use if no value is specified | ||
| * @param commaDelimited Whether to try to parse a string as a comma-delimited value | ||
| * @return The setting array values | ||
| */ | ||
| public String[] getAsArray(String settingPrefix, String[] defaultArray, Boolean commaDelimited) throws SettingsException { | ||
| public String[] getAsArray(String key, String[] defaultArray, Boolean commaDelimited) throws SettingsException { |
There was a problem hiding this comment.
If we are still going to have getAsArray, maybe we should store as an array instead of List? Then this method is really just a check + cast.
There was a problem hiding this comment.
so my plan was rather to change the return type in a followup to List<String> since I really don't want to return a mutable object. Makes sense?
There was a problem hiding this comment.
It does, I had not thought about the mutability of an array!
| @@ -468,21 +468,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 | |||
There was a problem hiding this comment.
these comments are no longer relevant i think? The essentially part was because each list element was it's own settings value?
There was a problem hiding this comment.
yeah correct I will remove it
s1monw
added a commit
that referenced
this pull request
Oct 5, 2017
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
tlrx
added a commit
that referenced
this pull request
Oct 10, 2017
List settings are not correctly filtered out in 5.6 and 6.0. This has been detected by a failing packaging test after #26878 has been merged.
tlrx
added a commit
that referenced
this pull request
Oct 10, 2017
List settings are not correctly filtered out in 5.6 and 6.0. This has been detected by a failing packaging test after #26878 has been merged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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