-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add deprecation warning for default value of action.destructive_requires_name
#70932
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 deprecation warning for default value of action.destructive_requires_name
#70932
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java
Outdated
Show resolved
Hide resolved
rjernst
left a comment
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.
Thanks William. I left a couple comments. Also, could we have some unit tests for the deprecation in DestructiveOperationsTests?
|
|
||
| public DestructiveOperations(Settings settings, ClusterSettings clusterSettings) { | ||
| destructiveRequiresName = REQUIRES_NAME_SETTING.get(settings); | ||
| isDestructiveRequiresNameSet = REQUIRES_NAME_SETTING.exists(settings); |
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.
Since the setting is updatable, can't this get out of sync?
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.
This is a little bit tricky, and I'm not sure how to address it. The DestructiveOperations object only has access to settings and clusterSettings at construction time, and there it sets up a listener for changes. That listener operates on a value for the setting, and doesn't know whether the value is pulled from the default or was explicitly provided. I've added a line to the callback that sets this value to true on any update to the action.destructive_requires_name property, so that form that point on we won't see deprecation warnings.
I think there are three options:
- What I've done here, which ought to log a warning up to the point where the setting is explicitly updated and not at all thereafter.
- Change the setting updater functions so that they can compare more than just values. (This seems very intrusive.)
- Update or override the
Setting#checkDeprecationmethod to log a deprecation warning when retrieving the new value from the raw settings.
I will take a look at doing 3 to see if it works.
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.
It doesn't look like 3 will be straightforward either, unless we make changes to Setting.
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.
There is an alternate settings update consumer method that may work. That is, instead of passing a consumer which takes in the value, there is a consumer which takes the entire settings object, but is only called when a fixed list of settings changes, in this case the one setting we care about. Then exists can be called on the settings object to determine whether this is set. Take a look at addSettingsUpdateConsumer(Consumer<Settings> consumer, List<? extends Setting<?>> settings)
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.
Works like a charm. Thank you for the help.
server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java
Outdated
Show resolved
Hide resolved
rjernst
left a comment
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
When we merge #66908, we need to add a breaking change warning for 7.x users who may be relying on the old default of the setting.
We're not deprecating the setting as a whole, just the old default value, but since we don't want anyone to be caught by surprise with this change, it seems appropriate to use the deprecation logger.
Relates #67543