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
Automatically add "index." prefix to the settings are changed on restore if the prefix is missing #10269
Automatically add "index." prefix to the settings are changed on restore if the prefix is missing #10269
Conversation
LGTM. I'm not to familiar with the snapshot and restore API, so I'd love a second reviewer to validate the context. One thing I was wondering is how a settings which isn't prefixed by "index." could end up in a snapshot. It seems that at least the current code changes any incoming requests to use the full syntax. |
@bleskes it's not for the settings in the snapshot, it's for the settings that you would like to modify during restore. For example, it's possible to create an index with |
} | ||
} | ||
updatedSettingsBuilder.put(request.settings()); | ||
updatedSettingsBuilder.remove("index"); |
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.
Why do we need to remove "index" here instead of just normalizing everything?
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's no longer needed. It was added to handle "index" parameter in the REST API but the rest API is removing this and other parameters for quite a while now. I will remove this line. Thanks!
Can you move all of the hardcoded Other than that and the question I asked, LGTM. |
@imotov I also think it may be good to backport this to 1.5 for the 1.5.1 release |
…ore if the prefix is missing Closes elastic#10133
e6ef6c9
to
96f4c06
Compare
Closes #10133