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

Validate max thread/merge settings #20383

Merged
merged 1 commit into from Sep 8, 2016
Merged

Validate max thread/merge settings #20383

merged 1 commit into from Sep 8, 2016

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Sep 8, 2016

This change checks that index.merge.scheduler.max_thread_count < index.merge.scheduler.max_merge_count and fails index creation
and settings update if the condition is not met.

Fixes #20380

This change checks that `index.merge.scheduler.max_thread_count` < `index.merge.scheduler.max_merge_count` and fails index creation
and settings update if the condition is not met.

Fixes #20380
@mikemccand
Copy link
Contributor

LGTM, thanks @jimferenczi

@s1monw
Copy link
Contributor

s1monw commented Sep 8, 2016

this LGTM, I wonder if we should do something about this in 2.x as well. We should at least try to not fail write request when settings fail to apply? Catching exceptions in onSettingsChanged and log it just to make sure we don't fail anything or cause weird sideeffects?

@jimczi jimczi merged commit 5739a7c into elastic:master Sep 8, 2016
@jimczi jimczi deleted the max_merge_settings branch September 8, 2016 10:47
@jimczi jimczi added the v2.4.1 label Sep 8, 2016
@jimczi
Copy link
Contributor Author

jimczi commented Sep 8, 2016

Thanks @mikemccand and @s1monw
I'll merge in 2.4.1 with the solution proposed by Simon.

s1monw pushed a commit that referenced this pull request Sep 8, 2016
s1monw pushed a commit that referenced this pull request Sep 8, 2016
jimczi added a commit that referenced this pull request Sep 8, 2016
Manual backport of #20383
Since settings are not validate in 2.x, this change catch the exception if the merge settings are not valid and do not apply them in the current CMS.
The downside is that we'll try to apply the new settings if the index is reopened or if the node is restarted, this will fail and the only solution for the user is to re-update the merge settings back to valid values.

Relates #20380
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants