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

Fix merge scheduler config settings #23391

Merged
merged 2 commits into from Feb 28, 2017

Conversation

Projects
None yet
3 participants
@jimczi
Member

jimczi commented Feb 27, 2017

This change fixes the retrieval of the merge scheduler config that previously used IndexSettings#getValue and failed to
retrieve the current value of the max_thread_count when max_merge_count was set.
Instead we now use Setting#get(Settings) which correctly retrieves the value (and default value) for all settings.
This fixes a bug that appears when only max_thread_count was updated. Any value for the setting was accepted leading to a state where the
merge scheduler throws exceptions (and fails any merge) and the index cannot be opened anymore (if it was closed before).

Fix merge scheduler config settings
This change fixes the retrieval of the merge scheduler config that previously used IndexSettings#getValue and failed to
retrieve the current value of the max_thread_count when max_merge_count was set.
Instead we now use Setting#get(Settings) which correctly retrieves the value (and default value) for all settings.
This fixes a bug that appears when only max_thread_count was updated. Any value for the setting was accepted leading to a state where the
 merge scheduler throws exceptions (and fails any merge) and the index cannot be opened anymore (if it was closed before).
@rjernst

I'm confused on what the actual issue was here. The only difference between what you have done here, and what existed with getValue() was using the raw settings in IndexSettings, vs the scopedSettings. And the latter is what is still used for updating. Is this actually a problem in AbstractScopedSettings?

@@ -69,8 +69,9 @@
private volatile int maxMergeCount;
MergeSchedulerConfig(IndexSettings indexSettings) {

This comment has been minimized.

@rjernst

rjernst Feb 27, 2017

Member

Can this class be unit tested instead of depending on integration tests?

@jbbarth

This comment has been minimized.

jbbarth commented Feb 27, 2017

@rjernst if it helps reproducing the problem, I think this PR aims at fixing a problem I reported on the forum here: https://discuss.elastic.co/t/problem-updating-settings-maxthreadcount-8-should-be-maxmergecount-6/76279

After review
Change Setting#get(Settings, Settings) to fallback only if the setting is present in the secondary.
Replace IT with uts.
@rjernst

Thanks for the new tests!! And also for tracking down the root cause. I have one suggestion.

@@ -378,7 +377,7 @@ public final T get(Settings primary, Settings secondary) {
return get(primary);
}
if (fallbackSetting == null) {
return get(secondary);
return exists(secondary) ? get(secondary) : get(primary);

This comment has been minimized.

@rjernst

rjernst Feb 28, 2017

Member

I think this block and the following one could be reordered to get the same effect?

if (exists(secondary)) {
    return get(secondary);
}
if (fallbackSetting == null) {
    return get(primary);
}

@jimczi jimczi merged commit d27a558 into elastic:master Feb 28, 2017

1 of 2 checks passed

abs-temporary-test Build finished.
Details
CLA Commit author is a member of Elasticsearch
Details

@jimczi jimczi deleted the jimczi:merge_scheduler_setting branch Feb 28, 2017

jimczi added a commit that referenced this pull request Feb 28, 2017

Fix merge scheduler config settings (#23391)
Change Setting#get(Settings, Settings) to fallback only if the setting is present in the secondary.
This is needed to fix setting that relies on other settings.
Replace IT with uts.

jimczi added a commit that referenced this pull request Feb 28, 2017

Fix merge scheduler config settings (#23391)
Change Setting#get(Settings, Settings) to fallback only if the setting is present in the secondary.
This is needed to fix setting that relies on other settings.
Replace IT with uts.
@jimczi

This comment has been minimized.

Member

jimczi commented Feb 28, 2017

Thanks @rjernst and @jbbarth !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment