Allow sharing one merge thread pool across IndexWriters#2998
Allow sharing one merge thread pool across IndexWriters#2998marcbachmann wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f9dd53bed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let merge_thread_pool = match shared_merge_pool { | ||
| Some(shared_pool) => shared_pool, | ||
| None => { |
There was a problem hiding this comment.
When shared_merge_pool is supplied, this path accepts an arbitrary Rayon pool and skips the panic handler that Tantivy installs on its private merge pool. start_merge catches merge panics, sends the FutureResult error, and then resumes unwinding with PANIC_CAUGHT; on a normal user-created Rayon pool with no custom panic_handler, that resumed panic from a spawned task uses Rayon's default unobserved-panic behavior and can abort the whole process instead of reporting a failed merge. This affects any shared-pool writer when a merge panics, including the same scenarios the existing private-pool handler was added to tolerate.
Useful? React with 👍 / 👎.
The merge thread caught a panic, reported it through the future, then re-raised it via resume_unwind so the private pool's panic_handler could swallow it. A user-supplied shared_merge_pool has no such handler, so the re-raised panic escaped into rayon's spawn and aborted the whole process instead of surfacing a failed merge. Report the panic through the future and return without re-raising, so merge panics are contained regardless of which pool runs them. The PANIC_CAUGHT sentinel and its handler branch are no longer needed.
A directory wrapper panics on open_write once armed, forcing a panic inside a merge running on a user-supplied shared pool (which has no rayon panic handler). The test asserts the panic surfaces as a SystemError through the merge future. With the previous resume_unwind behavior this test dies with SIGABRT instead of failing, so it directly guards the regression.
1 participant
Problem
Every
IndexWriterbuilds a private rayon pool ofnum_merge_threads(default 4) merge threads. A process hosting many indexes — one writer per
tenant — ends up with
4 × open_writersmerge threads and no way to boundmerge concurrency machine-wide (64 open writers → 256 merge threads).
Fix
When set,
num_merge_threadsis ignored and theSegmentUpdaterspawnsmerge work on the shared pool instead of a private one. Writers without the
option behave exactly as before (private pool), so the change is opt-in and
backward compatible. Covered by
test_shared_merge_pool_across_indexwriters.