[Draft] Multi-Tenant CMS Manager #14953
Conversation
Initial working MultiTenantCMSManager with 3 basic tests
A simplistic MultiTenantCMSManager implementation
A simplistic MultiTenantCMSManager implementation with 3 basic tests
add dynamic rebalancing and live writer tests
merge main CMS manager
merge CMS manager edits and tests
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
|
Continuing where @yaser-aj left off, here's what I plan to tackle next:
Please let us know if we are missing anything important. Thanks! |
There was a problem hiding this comment.
Thanks Yaser, and Luke, for sharing a prototype of this approach. The simplicity of tweaking things from outside while leaving CMS untouched, is indeed tempting!
I do agree with @jpountz however that
- The simplest version (equal sharing) is too limited.
- As we start building more elaborate logic, things get very complex very quickly.
- As a small example, an equal sharing strategy doesn't account for some schedulers getting more merges than others. Once you start doing that,
updateBudget()must be called each time a merge is submitted or completed, which needs deeply integrated hooks. - The real complexity in designing and maintaining, lies in figuring out the right policy budget allocation policy.
- As a small example, an equal sharing strategy doesn't account for some schedulers getting more merges than others. Once you start doing that,
- It doesn't provide a sure way of controlling the total no. of threads consumed by indexing+merging, which is a primary motivation for such a scheduler.
Hence, I'm leaning more in the direction of #14900, at least as of now. Happy to hear more thoughts on this.
Regardless, there's a lot of good work here, especially in the tests, which can be leveraged in any impl. we pursue for this change.
| * | ||
| * @param cms the merge scheduler to unregister | ||
| */ | ||
| public void unregister(ConcurrentMergeScheduler cms) { |
There was a problem hiding this comment.
nit: should we call it "deregister"?
| int count = schedulers.size(); | ||
| if (count == 0) return; | ||
|
|
||
| int share = Math.max(1, maxThreadCount / count); |
There was a problem hiding this comment.
This should be Math.min(), otherwise, we'll set some schedulers to 0 maxMergeThreads.
| ConcurrentMergeScheduler[] cmsArr = new ConcurrentMergeScheduler[numSchedulers]; | ||
| for (int i = 0; i < numSchedulers; i++) { | ||
| cmsArr[i] = new ConcurrentMergeScheduler(); | ||
| manager.register(cmsArr[i]); |
There was a problem hiding this comment.
We already register in the CMS ctor. Do we need to register again?
|
|
||
| /** Used in tests to read the current registered CMS set */ | ||
| synchronized Set<ConcurrentMergeScheduler> getRegisteredSchedulersForTest() { | ||
| return new HashSet<>(schedulers); |
There was a problem hiding this comment.
Maybe just return a 1Collections.unmodifableSet()` ? and avoid allocating a new set and copy all the entries into it..
| } | ||
|
|
||
| /** Used in tests to clear all registered CMS instances */ | ||
| synchronized void unregisterAllForTest() { |
There was a problem hiding this comment.
Why does this method need to be synchronized ?
A proposed solution to #13883.
Currently, each
ConcurrentMergeSchedulermanages its own threads independently, assuming no other merge schedulers are running in the JVM. This leads to potential over-allocation of system resources when manyIndexWriters are active. This draft PR demonstrates a simple approach to multi-tenancy by introducing a global manager that divides a global thread budget among all registered CMS instances.Key Changes
MultiTenantCMSManagerConcurrentMergeSchedulerinstances.maxThreadCountandmaxMergeCountas CMSs are registered or unregistered.ConcurrentMergeSchedulerIndexWriterintegration, and auto-unregistering behavior.Important Notes
Will update this draft PR with more points + to-dos soon.