Move all dynamic settings and their config classes to the index level #15955
Conversation
Today we maintain a lot of settings on the shard level which are all index level settings. In order to cut over to the new settings API where we register update listener we have to move all of them on to the index level otherwise we need a way to un-register listeners which is error-prone and requires additional handling when shards are closed. It's simpler and also more accurate to handle all of them on the index level where we can trash the entire registry for update listener once the index goes out of scope.
s1monw
added
>enhancement
review
:Core/Infra/Settings
|
LGTM |
| * Index setting to control if a flush is executed before engine is closed | ||
| * This setting is realtime updateable. | ||
| */ | ||
| public static final String INDEX_FLUSH_ON_CLOSE = "index.flush_on_close"; |
There was a problem hiding this comment.
Why would a user set this setting to false ? It's not taken into account when the shard is closed due to relocations or when the index is removed so is it useful to have it as an updatable index setting ? IMO this could be a chance to remove the setting and force the value to true.
There was a problem hiding this comment.
I like keeping changes like this purely mechanical though. If we should remove the setting we can remove it after I think.
There was a problem hiding this comment.
Sure I was just wondering if there is a use case for this.
There was a problem hiding this comment.
we use this setting mainly in tests but I might be able to work around that. it totally doesn't need to be updateable. I will do this in a followup
|
LGTM. @jimferenczi has a point about maybe removing INDEX_FLUSH_ON_CLOSE, probably worth its own issue. Left a silly comment about naming that can come later/never. |
Move all dynamic settings and their config classes to the index level
`index.flush_on_close` is a test setting and doesn't need to be updateable. Relates to elastic#15955
Today we maintain a lot of settings on the shard level which are all index level settings.
In order to cut over to the new settings API where we register update listener we have to move
all of them on to the index level otherwise we need a way to un-register listeners which is error-prone
and requires additional handling when shards are closed. It's simpler and also more accurate to handle all of
them on the index level where we can trash the entire registry for update listener once the index goes out of scope.
Related to #6732