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

Remove SerialMergeScheduler #6120

Closed
mikemccand opened this issue May 11, 2014 · 5 comments
Closed

Remove SerialMergeScheduler #6120

mikemccand opened this issue May 11, 2014 · 5 comments
Assignees
Labels

Comments

@mikemccand
Copy link
Contributor

In master we can just remove it, and for 1.2 I think we should "translate" it into the equivalent ConcurrentMergeScheduler (max_thread_count=1, max_merge_count=1).

This merge scheduler is not practical for real usage: it only allows one merge to run at a time, and it's scync'd so it blocks any other incoming threads.

It also doesn't work with the upcoming index throttling (https://github.com/elasticsearch/elasticsearch/tree/enhancement/throttle_engine ), and while we could "solve" this by just documenting that apps shouldn't use SMS, I'd prefer to remove/remap it instead.

@mikemccand mikemccand self-assigned this May 11, 2014
@clintongormley
Copy link

++

@mikemccand
Copy link
Contributor Author

OK I pushed a fix for 1.x to this branch: https://github.com/elasticsearch/elasticsearch/tree/fix/6120

I would appreciate it if someone could review! This code is all new to me.

I just changed SMSProvider to secretly return its own CMS instance and set max_merge_threads and max_merge_count to 1. And I removed TrackingCMS, and "fixed" the docs to not mention SMS anymore. These changes are for 1.x ... for master I'll remove SMS entirely.

@jpountz
Copy link
Contributor

jpountz commented May 12, 2014

The change looks good to me, I just left minor comments. I think it's nice to remap the merge scheduler this way, the good news being that since merge schedulers are randomized in the integration tests, this compatibility layer should get pretty good test coverage.

@mikemccand
Copy link
Contributor Author

Thanks Adrien, I committed a fix for your feedback. I think it's ready! I'll commit soon, and then merge to master by removing SMS entirely.

@jpountz
Copy link
Contributor

jpountz commented May 12, 2014

+1

mikemccand added a commit that referenced this issue May 12, 2014
…r with max_thread_count=1 (remove in master)

It's dangerous to expose SerialMergeScheduler as an option: since it only allows one merge at a time, it can easily cause merging to fall behind.
@clintongormley clintongormley added >enhancement :Core/Infra/Core Core issues without another label labels Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants