Skip to content
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

Merged
merged 1 commit into from Mar 30, 2015

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Mar 26, 2015

Closes #10133

@imotov imotov added >bug v2.0.0-beta1 review :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v1.6.0 labels Mar 26, 2015
@bleskes
Copy link
Contributor

bleskes commented Mar 26, 2015

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.

@imotov
Copy link
Contributor Author

imotov commented Mar 26, 2015

@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 index.refresh_interval setting as well as simply refresh_interval. The result is the same. However, without this change if you will try to change the refresh_interval in the later way (without index.) this change will be simply ignored. Basically this test fails.

}
}
updatedSettingsBuilder.put(request.settings());
updatedSettingsBuilder.remove("index");
Copy link
Member

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?

Copy link
Contributor Author

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!

@dakrone
Copy link
Member

dakrone commented Mar 30, 2015

Can you move all of the hardcoded "index." strings into a static IndexMetaData.INDEX_SETTING_PREFIX? Makes it much less likely that we will typo something in the future.

Other than that and the question I asked, LGTM.

@dakrone
Copy link
Member

dakrone commented Mar 30, 2015

@imotov I also think it may be good to backport this to 1.5 for the 1.5.1 release

@imotov imotov force-pushed the issue-10133-index-prefix-in-restore branch from e6ef6c9 to 96f4c06 Compare March 30, 2015 22:53
@imotov imotov added the v1.5.1 label Mar 30, 2015
@imotov imotov merged commit 96f4c06 into elastic:master Mar 30, 2015
@clintongormley clintongormley changed the title Automatically add "index." prefix to the settings are changed on restore... Automatically add "index." prefix to the settings are changed on restore if the prefix is missing Jun 8, 2015
@imotov imotov deleted the issue-10133-index-prefix-in-restore branch May 1, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v1.5.1 v1.6.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restore doesn't prefix restore index settings with index.
4 participants