Make Settings Diffable #88815
Make Settings Diffable #88815
Conversation
Making settings diffable so that index metadata diffs are smaller whenever the metadata changes without a setting change as well as to make index setting updates over a wider number of indices faster. This saves about 3% of CPU time on master and about half that on data nodes that is just burnt for writing setting strings when bootstrapping many shards benchmarks benchmarks to 50k indices.
original-brownbear
added
>non-issue
:Core/Infra/Settings
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -402,6 +402,25 @@ public void writeTo(StreamOutput out) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| private static <K, T> Map.Entry<K, T> mapEntry(K key, T newValue) { | |||
There was a problem hiding this comment.
Just a quick fix here to enable null values as we can have them for settings. The default map entries don't allow null values.
| @@ -1420,7 +1433,13 @@ public void writeTo(StreamOutput out) throws IOException { | |||
| out.writeVLong(aliasesVersion); | |||
| } | |||
| out.writeByte(state.id); | |||
| Settings.writeSettingsToStream(settings, out); | |||
| assert settings != null | |||
There was a problem hiding this comment.
Not the nicest way to go about BwC but I couldn't see a neater solution.
As we never write this over cross-cluster connections this should be safe as far as I can tell.
|
Adding WIP label here, as I just realized I forgot dedicated test for the diffing. Will add shortly, please hold off on reviewing. |
| @@ -402,6 +402,25 @@ public void writeTo(StreamOutput out) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| private static <K, T> Map.Entry<K, T> mapEntry(K key, T newValue) { | |||
There was a problem hiding this comment.
Why the special map entry? These will be recreated by any map impls right?
There was a problem hiding this comment.
Yea, we really didn't need Map.Entry for the sites that we use it to begin with, Tuple would've been just fine but I didn't want to mix more refactoring than necessary into this PR so I went with this since weirdly enough I couldn't find a built-in JDK way of creating a Map.Entry that allows me a null value ...
| out.writeMap(settings, StreamOutput::writeString, Settings::writeSettingValue); | ||
| } | ||
|
|
||
| private static void writeSettingValue(StreamOutput streamOutput, Object value) throws IOException { |
There was a problem hiding this comment.
I know this was already here, and I think it was from a previous PR, but why aren't we using writeGenericValue?I do notice that we use the fully generic read above. If it's for performance, can you add a comment so that me or anyone else that wonders this in the future remembers. :)
There was a problem hiding this comment.
If it's for performance
yes that's what it's for, the difference to the generic method is massive here. Will add a comment
| @@ -441,6 +442,17 @@ public void testWriteSettingsToStream() throws IOException { | |||
| assertEquals(Arrays.asList("1", "2"), settings.getAsList("test.key4.foo")); | |||
| } | |||
|
|
|||
| public void testDiff() throws IOException { | |||
There was a problem hiding this comment.
Can we have some more tests? eg a noop test, a test with more than one setting, etc. Basically, can we have something to exercise each path in the setting specific diff code you added (maybe the noop is the only additional thing needed).
|
Jenkins run elasticsearch-ci/packaging-tests-unix-sample |
|
Thanks Ryan! Added a noop test and a list test case to cover all possible paths. |
Making settings diffable so that index metadata diffs are smaller whenever the metadata changes without a setting change as well as to make index setting updates over a wider number of indices faster. This saves about 3% of CPU time on master and about half that on data nodes that is just burnt for writing setting strings when bootstrapping many shards benchmarks benchmarks to 50k indices.
Follow-up to elastic#88815. No need to have two equivalent implementations here.
Follow-up to elastic#88815. No need to have two equivalent implementations here.
Making settings diffable so that index metadata diffs
are smaller whenever the metadata changes without a setting change
as well as to make index setting updates over a wider number of
indices faster.
This saves about 3% of CPU time on master and about half that on data nodes
that is just burnt for writing setting strings (mainly a result of the extremely heavy default query fields list in the Beats template I think) when bootstrapping many shards benchmarks benchmarks to 50k indices.
relates #77466