Use latest setting value when initializing setting watch #134091
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
When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
rjernst
added
>bug
:Core/Infra/Settings
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @rjernst, I've created a changelog YAML for you. |
ldematte
reviewed
Sep 8, 2025
| assert setting.getProperties().contains(Setting.Property.NodeScope) : "Can only watch node settings"; | ||
| consumer.accept(setting.get(settings)); | ||
|
|
||
| // this mimics the combined settings of last applied and node settings, without building a new settings object |
There was a problem hiding this comment.
With this comment I understand you are trying to avoid doing
Settings.builder().put(this.settings).put(this.lastSettingsApplied).build() like in other functions, correct?
Is there an advantage? Is it correct? (is lastSettingsApplied always a superset of settings?)
There was a problem hiding this comment.
Building a combined Settings object is heavyweight. It requires combing the internal maps that each Settings object contains.
If the latter settings object contains a setting we are looking for, it would have overridden the value when combined, so looking at the latter first should mimic the combined behavior.
There was a problem hiding this comment.
Building a combined Settings object is heavyweight. It requires combing the internal maps that each Settings object contains.
I understand that, but I was wondering if it really matters in this case - I might be mistaken, but this is an initialization method, so probably it's not on a hot path.
As long as it's correct I like the optimization though.
There was a problem hiding this comment.
this is an initialization method
While true, it's also called for every setting that is watched, currently 43 uses. Seems silly to reconstruct the same settings object 43 times (but also not worth the complexity to cache it in a threadsafe way).
ldematte
approved these changes
Sep 8, 2025
This was referenced Sep 8, 2025
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Sep 8, 2025
) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Sep 8, 2025
) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Sep 8, 2025
) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Sep 8, 2025
) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 8, 2025
…134315) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes #133701
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 8, 2025
…134314) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes #133701
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 8, 2025
…134313) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes #133701
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Sep 9, 2025
) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 10, 2025
…134316) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes #133701
sarog
pushed a commit
to portsbuild/elasticsearch
that referenced
this pull request
Sep 11, 2025
) (elastic#134316) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
sarog
pushed a commit
to portsbuild/elasticsearch
that referenced
this pull request
Sep 19, 2025
) (elastic#134316) When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists. closes elastic#133701
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
When new cluster settings are applied, consumers watchings are notified. The consumer should see the latest value at the time of initializing a watch. However, only the node settings were ever used in finding the value to initialize the consumer. This commit fixes the consumer to use the value from latest applied if it exists.
closes #133701