-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Adding a deprecation info API check for fractional byte value settings #77074
Adding a deprecation info API check for fractional byte value settings #77074
Conversation
Pinging @elastic/es-data-management (Team:Data Management) |
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
Left some comments, but not sure if any of them are particularly actionable. I wish there were a good/easy way to specifically target the settings that expect byte values, but this should do well enough.
Map<String, String> fractionalByteSettings = new HashMap<>(); | ||
for (String key : settings.keySet()) { | ||
try { | ||
settings.getAsBytesSize(key, ByteSizeValue.ZERO); |
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 looks like it triggers deprecation logging as well for each fractional value read, do we want that?
if (stringValue.contains(".")) { | ||
fractionalByteSettings.put(key, stringValue); | ||
} | ||
} catch (Exception ignoreThis) { |
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.
Hmm, I'm not a huge fan of Exception based control flow. I know that settings are registered with ClusterSettings and IndexScopedSettings so that settings can be validated against the master set, but I don't know if we can filter on the setting's data type due to type erasure, and I'm not sure if those are comprehensive for every setting either.
@elasticmachine update branch |
In 6.2.0 we deprecated support for fractional byte value settings. Support was originally going to be
removed altogether in 8.0.0 (see #53927) but that PR was not merged. This commit adds a warning
deprecation info check if any fractional byte values settings are found.
Relates #42404