-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix enrich cache size setting name #117575
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
Conversation
The enrich cache size setting accidentally got renamed from `enrich.cache_size` to `enrich.cache.size` in elastic#111412. This commit reverts that rename. The fix that gets backported to 8.16, 8.17 and 8.x will allow both versions, to avoid breaking BWC twice.
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @nielsbauman, I've created a changelog YAML for you. |
| * This setting solely exists because the original setting was accidentally renamed in | ||
| * https://github.com/elastic/elasticsearch/pull/111412. | ||
| */ | ||
| @UpdateForV10(owner = UpdateForV10.Owner.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.
Are the Settings.Property.Deprecated below and this @Update too soon?
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 the @UpdateForV10 is fine, but the deprecated thing I'm not sure about yet. What I don't like about it here is that we can't redirect users back to the setting they should be using (cache_size). Perhaps it'd be worth adding it into the constructor rather than the setting itself? Both is probably fine (the annotation would let us see it in the migration APIs I think)
dakrone
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, I left one more comment, I also think we should put a deprecation log message if the BWC name is used that points the user to the non-deprecated version (otherwise we just say "this is deprecated" without telling the user how to fix it)
x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPlugin.java
Show resolved
Hide resolved
dakrone
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, I left one more comment (hopefully not hard?)
x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPlugin.java
Outdated
Show resolved
Hide resolved
The enrich cache size setting accidentally got renamed from `enrich.cache_size` to `enrich.cache.size` in elastic#111412. This commit updates the enrich plugin to accept both names and deprecates the wrong name.
The enrich cache size setting accidentally got renamed from `enrich.cache_size` to `enrich.cache.size` in elastic#111412. This commit updates the enrich plugin to accept both names and deprecates the wrong name.
The enrich cache size setting accidentally got renamed from `enrich.cache_size` to `enrich.cache.size` in elastic#111412. This commit updates the enrich plugin to accept both names and deprecates the wrong name.
The enrich cache size setting accidentally got renamed from
enrich.cache_sizetoenrich.cache.sizein #111412. This commit updates the enrich plugin to allow both names and deprecates the wrong one.