-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a property to mark setting as final #23872
Conversation
This change adds a setting property that sets the value of a setting as final. Updating a final setting is prohibited in any context, for instance an index setting marked as final must be set at index creation and will refuse any update even if the index is closed.
@@ -96,6 +96,11 @@ | |||
Dynamic, | |||
|
|||
/** | |||
* mark this setting as final (the value cannot be updated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth explaining here how this is different than not marking the setting as Dynamic
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're looking to prevent updates when an index is closed then we might want to have a setting for that too. That way we can (eventually) require that all settings have one of the three.
I don't like not having one of these meaning "can only be updated on an index and only when that index is closed". If we have three update scenarios we may as well have three properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth explaining here how this is different than not marking the setting as Dynamic.
The value cannot be updated in non-dynamic context ? ;)
My understanding is that Dynamic
means a setting that can be updated dynamically.
This does not prevent this setting to be updated non-dynamically so I think that Final
makes sense as an abstract property in the setting object. Non dynamic contexts are defined at the scope level, so for instance updating the settings on a closed index is considered as non-dynamic. And if we want to add more non-dynamic context we should just ensure that this property is applied ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah just put this explain part in the comment. we can be explicit and say that it also prevents updates if the index is closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I left 2 suggestions... I also think we should make index.number_of_shards
in IndexMetaData
final that way we can remove the special casing for it in MetaDataUpdateSettingsService#updateSettings
I hope there is a test for this already
@@ -388,6 +388,14 @@ public boolean hasDynamicSetting(String key) { | |||
} | |||
|
|||
/** | |||
* Returns <code>true</code> if the setting for the given key is final. Otherwise <code>false</code>. | |||
*/ | |||
public boolean hasFinalSetting(String key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we name it isSettingFinal
?
@@ -96,6 +96,11 @@ | |||
Dynamic, | |||
|
|||
/** | |||
* mark this setting as final (the value cannot be updated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah just put this explain part in the comment. we can be explicit and say that it also prevents updates if the index is closed
This change adds a setting property that sets the value of a setting as final. Updating a final setting is prohibited in any context, for instance an index setting marked as final must be set at index creation and will refuse any update even if the index is closed. This change also marks the setting `index.number_of_shards` as Final and the special casing for refusing the updates on this setting has been removed.
* master: Fix Javadocs for BootstrapChecks#enforceLimits Disable bootstrap checks for single-node discovery Add unit tests for the missing aggregator (elastic#23895) Add a property to mark setting as final (elastic#23872) testDifferentRolesMaintainPathOnRestart - fix broken comment testDifferentRolesMaintainPathOnRestart - lower join timeout as split elections are likely Introduce single-node discovery Await termination after shutting down executors
This change adds a setting property
Final
that sets the value of a setting as final.Updating a
Final
setting is prohibited in any context, for instance an index settingmarked as final must be set at index creation and will refuse any update even if the index is closed.