Optimize ConcurrentMergeScheduler for Multi-Tenant Indexing #14335

DivyanshIITB · 2025-03-07T19:00:42Z

This PR enhances the ConcurrentMergeScheduler by introducing dynamic resource allocation for multi-tenant indexing scenarios.

Key Improvements:

Global Counter for Active IndexWriters: Introduced an AtomicInteger to track active IndexWriter instances dynamically.
Dynamic Thread Allocation: Adjusted maxThreadCount based on the number of active writers to optimize merge performance.
Adaptive Merge Throttling: Distributed merge speed limits among active writers to prevent resource contention and ensure fair indexing performance.

These changes improve efficiency in multi-writer environments by dynamically adapting merge scheduling to system load.

Fixes #13883

jpountz · 2025-03-08T06:37:59Z

This looks too naive to me, we don't want each index writer to have 1/N of the resources, which would prevent one writer from maxing out resources (e.g. if one index has a heavy write load while other indexes are only rarely updated).

DivyanshIITB · 2025-03-10T13:22:28Z

Thank you for the review, @jpountz!

I see your concern regarding equal resource distribution across IndexWriter instances potentially leading to inefficiencies when some writers have a significantly heavier workload than others. My initial goal was to ensure fairness in multi-tenant environments, but I agree that a static 1/N allocation might not be ideal.

To address this, I’m considering an adaptive resource allocation approach, where:

Recent Merge Activity Tracking: Each IndexWriter's recent merge activity is monitored.
Dynamic Thread Allocation: Instead of an equal split, threads are assigned based on recent workload.
Merge Throttling Balance: Prevents a single heavy writer from starving others while ensuring underutilized writers don’t hold
excessive resources.

Would this approach align better with Lucene’s design goals?

jpountz · 2025-03-11T16:29:03Z

Merge throttling is now disabled by default, IMO it's fine to ignore merge throttling for now. Regarding thread creation, I'm thinking of a shared fixed (e.g. number of processors / 2) thread pool for merge scheduling.

DivyanshIITB · 2025-03-11T16:51:27Z

Thank you for the clarification, @jpountz!

I'll drop the merge throttling aspect from the changes since it's disabled by default.
Regarding the fixed thread pool approach (numProcessors / 2), should this be a shared global pool across all IndexWriters, or should each writer have its own pool?
If it's a shared pool, how should we handle cases where a few writers are highly active while others are idle? Should we allow active writers to take more resources dynamically, or keep a strict fixed allocation?
Let me know your thoughts, and I'll adjust the implementation accordingly!

DivyanshIITB · 2025-03-14T11:28:11Z

Just a gentle reminder

jpountz · 2025-03-14T14:45:04Z

Apologies I had missed your reply.

should this be a shared global pool across all IndexWriters, or should each writer have its own pool?

It should be shared, we don't want the total number of threads to scale with the number of index writers. The reasoning for the numProcessors/2 number is that merging generally should not be more expensive than indexing, so by reserving only half the CPU capacity for merging, it should still be possible to max out hardware while indexing, while also having a peak number of threads running merges under numProcessors/2.

If it's a shared pool, how should we handle cases where a few writers are highly active while others are idle? Should we allow active writers to take more resources dynamically, or keep a strict fixed allocation?

Idle writers would naturally submit fewer tasks than highly active writers. IMO the fixed allocation is key here.

DivyanshIITB · 2025-03-15T06:25:12Z

Thanks for the detailed clarification, @jpountz!

I have made the necessary changes to the implementation:

Implemented a shared global thread pool with a fixed size of numProcessors / 2.
Ensured that each IndexWriter submits merge tasks to the shared pool without dynamic reallocation, as idle writers naturally
submit fewer tasks.
Updated the logic in MergeScheduler.java to align with these changes.

@jpountz, I’ve made the requested changes. Please review when you get a chance.

jpountz · 2025-03-15T21:08:09Z

I think it'll be simpler to create a new merge scheduler rather than modify ConcurrentMergeScheduler. Also we'll need tests.

DivyanshIITB · 2025-03-16T05:55:31Z

Thanks for your feedback, @jpountz!

I have created a new MultiTenantMergeScheduler as suggested, instead of modifying ConcurrentMergeScheduler.
I have restored ConcurrentMergeScheduler to its original state.
Added a test case (TestMultiTenantMergeScheduler) to validate the new scheduler's behavior.

Please review and let me know if any further changes are needed!

vigyasharma

@DivyanshIITB : I appreciate your interest in this issue. A multi-tenant CMS however, has quite a few nuances that this implementation will need to cover. I'd encourage you to look at the different ways in which IndexWriter interacts with merging. Some examples of functionality I find missing include support for forceMerging, aborting a merge, and waiting till all merges complete.

I can help review future iterations that include these changes.

vigyasharma · 2025-03-17T06:43:47Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+public class MultiTenantMergeScheduler extends MergeScheduler {
+
+    // Shared global thread pool
+    private static final ExecutorService MERGE_THREAD_POOL = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() / 2);


This will create a threadpool even when this policy is not used. Let's use lazy static initialization.

Thanks for the suggestion! I've updated the implementation to use lazy static initialization by wrapping the thread pool in a nested static class. This ensures the thread pool is only created when first accessed, avoiding unnecessary resource allocation. Let me know if you have any further feedback! Now, instead of directly initializing MERGE_THREAD_POOL, I'm using a LazyHolder class with a getMergeThreadPool() method to ensure thread-safe lazy initialization. This should prevent the thread pool from being created when the policy is unused."

vigyasharma · 2025-03-17T06:44:51Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+    public void merge(MergeScheduler.MergeSource mergeSource, MergeTrigger trigger) throws IOException {
+        while (true) {
+            MergePolicy.OneMerge merge = mergeSource.getNextMerge();
+            if (merge == null) {


Can we use hasPendingMerges() instead? I know existing schedulers assume null means there are no more merges, but it's not the documented API behavior.

Thanks for pointing this out! I’ve updated the loop condition to use hasPendingMerges() instead of relying on null. This ensures better adherence to the documented API behavior. Let me know if this aligns with what you were suggesting!

vigyasharma · 2025-03-17T06:52:28Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+            // Submit merge task to the shared thread pool
+            Future<?> future = MERGE_THREAD_POOL.submit(() -> {
+                try {
+                    mergeSource.merge(merge);
+                } catch (IOException e) {
+                    throw new RuntimeException(e);
+                }
+            });
+
+            try {
+                future.get(); // Ensure the task completes
+            } catch (Exception e) {
+                throw new IOException("Merge operation failed", e);
+            }


I don't think this achieves concurrent background merging like we'd want. I think this code will submit a merge request and block on its future for completion, effectively making all merges sequential.

Thanks for the feedback! You're absolutely right—blocking on future.get(); was making the merges sequential. I've removed it so that merges can now proceed asynchronously in the background. Let me know if this aligns with the intended behavior!

vigyasharma · 2025-03-17T07:06:07Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+    }
+
+    @Override
+    public void close() {


This shouldn't be a no-op. We should wait for all running merges of the calling IndexWriter to complete, similar to sync() in ConcurrentMergeScheduler. This could be done by maintaining a per writer list of futures for running merges. We could run a clean up loop in merge() that removes completed futures.

Also, I get that this thread pool is global and shared, but I don't like the idea of having a threadpool that can never be closed. How about we expose some hooks to close this scheduler if all consuming writers have closed?

Thanks for pointing this out! I’ve updated close() to properly wait for all running merges, similar to sync() in ConcurrentMergeScheduler. Now, merge() tracks active tasks, and I've added cleanup for completed futures. Also, I introduced shutdownThreadPool() to allow safe global shutdown. Let me know if this works!

vigyasharma · 2025-03-17T07:09:42Z

lucene/core/src/test/org/apache/lucene/index/TestMultiTenantMergeScheduler.java

+
+public class TestMultiTenantMergeScheduler extends LuceneTestCase {
+
+    public void testMultiTenantMergeScheduler() throws Exception {


This test is not sufficient. We need to test (and also implement) a lot more scenarios, like multiple writers consuming this scheduler, scheduling different merges and starting/closing at different times. You can refer to the tests in TestConcurrentMergeScheduler as a starting point.

Thanks for the feedback! I’ve expanded the test suite to include multiple writers consuming the scheduler, different merge scheduling times, and staggered start/close behavior. Also, I’ve referred to TestConcurrentMergeScheduler for structuring test cases to verify concurrency. Let me know if you have additional suggestions!

…-merge-scheduler-clean

…heduler.java

…tDeletionPolicy.java

DivyanshIITB · 2025-03-17T12:43:20Z

I have a request to you. Kindly ignore the following two deleted files in the "Files Changed" section :
"KeepOnlyLastCommitDeletionPolicy.java"
"ConcurrentMergeScheduler.java"

Thanks!

mccullocht · 2025-03-17T21:08:53Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+    }
+
+    @Override
+    public void close() throws IOException {


IIRC IndexWriter.close() will call MergeScheduler.close() which means that you would wait for all merges across all indexes using this scheduler to complete. The IndexWriter that is currently calling close() will stop scheduling new merges, but the other writers may not, which means this may not terminate. To fix this you may need to map active merges back to the IndexWriter or Directory they belong to.

+1; my suggestion in the previous review was similar. We need a mapping of writer to merges and only block on merges specific to the calling writer.

vigyasharma · 2025-03-18T07:01:15Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+    private static class LazyHolder {
+        static final ExecutorService MERGE_THREAD_POOL = 
+            Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() / 2);
+    }
+
+    private static ExecutorService getMergeThreadPool() {
+        return LazyHolder.MERGE_THREAD_POOL;
+    }
+
+    // Use getMergeThreadPool() instead of direct access


We can skip the getMergeThreadPool() function since we only need to access the threadpool internally. Something like:

private static class MergeThreadPool { private static final ExecutorService INSTANCE = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() / 2); } // and then just use MergeThreadPool.INSTANCE inside merge() and other functions. // ... }

vigyasharma · 2025-03-18T07:02:08Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+            });
+
+            // Cleanup completed merges
+            activeMerges.removeIf(Future::isDone);


Where is this declared and initialized? When are merges added to activeMerges ?

vigyasharma · 2025-03-18T07:03:30Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+    }
+
+    @Override
+    public void close() throws IOException {


+1; my suggestion in the previous review was similar. We need a mapping of writer to merges and only block on merges specific to the calling writer.

vigyasharma · 2025-03-18T07:06:59Z

lucene/core/src/test/org/apache/lucene/index/TestMultiTenantMergeScheduler.java

+        MultiTenantMergeScheduler.shutdownThreadPool();
+
+        // Check if merging took less time than sequential execution would
+        assertTrue("Merges did not happen concurrently!", (endTime - startTime) < 5000);


It's not reliable to assert or depend on elapsed time. Lucene is run of many different machines and this timing will differ.

vigyasharma

Hi @DivyanshIITB,

Thanks for your work here, a concurrent multi-tenant merge scheduler is an ambitious feature. This area of the codebase, especially dealing with how IndexWriter interacts with merging, can be quite complex.

I'd suggest the following steps to help things along:

First, please ensure your code compiles and all existing tests pass. You can run ./gradlew check to verify this.
A helpful approach would be to try swapping out the existing ConcurrentMergeScheduler (CMS) with your MultiTenantMergeScheduler (MTMS) in your local branch and running the test suite. This can reveal some subtle edge cases that your scheduler will need to handle.
You can use the existing tests for CMS as a guide for writing comprehensive tests for your MTMS.

This is a challenging task, and building familiarity with the codebase through smaller contributions can be very beneficial. Would you be interested in exploring some smaller, more focused issues to start?

Appreciate your effort and here to help.

vigyasharma · 2025-03-18T19:17:30Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+
+    @Override
+    public void close() throws IOException {
+        IndexWriter currentWriter = getCurrentIndexWriter();  // Method to get the calling writer


This is not defined anywhere!

vigyasharma · 2025-03-18T19:18:39Z

lucene/core/src/java/org/apache/lucene/index/MultiTenantMergeScheduler.java

+            merge.waitForCompletion(); // Only wait for merges related to this writer
+        }
+
+        activeMerges.remove(currentWriter); // Cleanup after closing


This doesn't line up, activeMerges is a List, you're calling Map APIs on it. Did this compile?

DivyanshIITB · 2025-03-19T09:25:53Z

Thankyou for your help @vigyasharma !
I would love to explore small and more focused issues to start with !

github-actions · 2025-04-03T00:24:23Z

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

DivyanshIITB added 3 commits March 3, 2025 02:23

Updated KeepOnlyLastCommitDeletionPolicy to retain last 5 commits

05d0125

Fix apache#13883: Multitenancy merge scheduler

de6f39b

Fix apache#13883: Multitenancy merge scheduler

5611150

github-project-automation bot added this to OpenSearch Lucene & Core Performance Tracking Mar 7, 2025

github-project-automation bot moved this to Open in OpenSearch Lucene & Core Performance Tracking Mar 7, 2025

github-actions bot added the module:core/index label Mar 7, 2025

Refactored merge thread management using a fixed-size thread pool

3a09834

Added MultiTenantMergeScheduler and test file

1f5660b

vigyasharma requested changes Mar 17, 2025

View reviewed changes

Merge branch 'fix-multitenancy-merge-scheduler' into fix-multitenancy…

6d0f03a

…-merge-scheduler-clean

DivyanshIITB force-pushed the fix-multitenancy-merge-scheduler-clean branch from 71ac56f to b6e7556 Compare March 17, 2025 12:29

Fix: Updated MultiTenantMergeScheduler based on PR review

13b914c

DivyanshIITB force-pushed the fix-multitenancy-merge-scheduler-clean branch from 6c356ff to 13b914c Compare March 17, 2025 12:33

DivyanshIITB added 2 commits March 17, 2025 18:07

Delete lucene/core/src/java/org/apache/lucene/index/ConcurrentMergeSc…

0ecced3

…heduler.java

Delete lucene/core/src/java/org/apache/lucene/index/KeepOnlyLastCommi…

00ee44c

…tDeletionPolicy.java

mccullocht reviewed Mar 17, 2025

View reviewed changes

vigyasharma reviewed Mar 18, 2025

View reviewed changes

DivyanshIITB added 2 commits March 18, 2025 15:45

Remove redundant getMergeThreadPool() function

2d96156

Made the necessary changes according to the reviews

61b7970

vigyasharma requested changes Mar 18, 2025

View reviewed changes

github-actions bot added the Stale label Apr 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize ConcurrentMergeScheduler for Multi-Tenant Indexing #14335

Optimize ConcurrentMergeScheduler for Multi-Tenant Indexing #14335

DivyanshIITB commented Mar 7, 2025

jpountz commented Mar 8, 2025

DivyanshIITB commented Mar 10, 2025

jpountz commented Mar 11, 2025

DivyanshIITB commented Mar 11, 2025

DivyanshIITB commented Mar 14, 2025

jpountz commented Mar 14, 2025

DivyanshIITB commented Mar 15, 2025

jpountz commented Mar 15, 2025

DivyanshIITB commented Mar 16, 2025 •

edited

Loading

vigyasharma left a comment

vigyasharma Mar 17, 2025

DivyanshIITB Mar 17, 2025

vigyasharma Mar 17, 2025

DivyanshIITB Mar 17, 2025

vigyasharma Mar 17, 2025

DivyanshIITB Mar 17, 2025

vigyasharma Mar 17, 2025

DivyanshIITB Mar 17, 2025

vigyasharma Mar 17, 2025

DivyanshIITB Mar 17, 2025

DivyanshIITB commented Mar 17, 2025 •

edited

Loading

mccullocht Mar 17, 2025

vigyasharma Mar 18, 2025

vigyasharma Mar 18, 2025

vigyasharma Mar 18, 2025

vigyasharma Mar 18, 2025

vigyasharma Mar 18, 2025

vigyasharma left a comment

vigyasharma Mar 18, 2025

vigyasharma Mar 18, 2025

DivyanshIITB commented Mar 19, 2025

github-actions bot commented Apr 3, 2025


		public class TestMultiTenantMergeScheduler extends LuceneTestCase {

		public void testMultiTenantMergeScheduler() throws Exception {

Optimize ConcurrentMergeScheduler for Multi-Tenant Indexing #14335

Are you sure you want to change the base?

Optimize ConcurrentMergeScheduler for Multi-Tenant Indexing #14335

Conversation

DivyanshIITB commented Mar 7, 2025

jpountz commented Mar 8, 2025

DivyanshIITB commented Mar 10, 2025

jpountz commented Mar 11, 2025

DivyanshIITB commented Mar 11, 2025

DivyanshIITB commented Mar 14, 2025

jpountz commented Mar 14, 2025

DivyanshIITB commented Mar 15, 2025

jpountz commented Mar 15, 2025

DivyanshIITB commented Mar 16, 2025 • edited Loading

vigyasharma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DivyanshIITB commented Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigyasharma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DivyanshIITB commented Mar 19, 2025

github-actions bot commented Apr 3, 2025

DivyanshIITB commented Mar 16, 2025 •

edited

Loading

DivyanshIITB commented Mar 17, 2025 •

edited

Loading