Reuse indexing worker threads across commits - #2999
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35f073f85e
ℹ️ 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".
| } else { | ||
| // Channel drained and closed: flush barrier reached. | ||
| break Ok(()); |
There was a problem hiding this comment.
Advance delete cursors on empty epochs
In epochs containing only delete operations (or no add batches), this branch acknowledges the epoch without moving the persistent worker's delete_cursor. Before this change the worker exited on each commit and the replacement cursor started after the delete-only commit; now the cursor can remain before those deletes indefinitely. If delete_all_documents() later rewinds opstamps and new matching docs are added, a subsequent commit can apply the stale delete to the new segment, making documents disappear even though the delete belonged to an earlier cleared generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I checked this against the parent commit before concluding anything, and the premise here turns out to be wrong: the replacement cursor did not start after a delete-only commit. DeleteQueue::cursor() positions at the end of the last flushed block, and delete operations pushed since the last consumption sit unflushed in the writer vec. The old code respawned workers inside prepare_commit(), i.e. before the segment updater consumed that commit's deletes, so a respawned worker's fresh cursor iterated those pending deletes exactly like the persistent cursor does now.
The disappearing-documents scenario is real, but pre-existing rather than introduced by this PR. Repro (fails identically on the parent commit and on this branch):
writer.add_document(doc!(text => "hello"))?;
writer.commit()?;
writer.delete_term(Term::from_field_text(text, "hello")); // uncommitted
writer.delete_term(Term::from_field_text(text, "hello")); // uncommitted
writer.delete_all_documents()?; // reverts the stamper
writer.commit()?;
writer.add_document(doc!(text => "hello"))?; // opstamp overlaps stale deletes
writer.commit()?;
// num_docs == 0 on both commits, expected 1The root cause is that delete_all_documents() reverts the stamper but never clears the delete queue, so uncommitted deletes survive with opstamps above the reverted stamp. Only uncommitted deletes are hazardous (committed ones always sit below the reverted committed_opstamp and get skipped by skip_to), and uncommitted deletes are unflushed — ahead of any cursor, old or new. No cursor-position strategy can fix that; it needs the queue to be cleared on delete_all_documents(). I'll file that separately.
There was one real regression hiding behind this observation though: a parked worker's persistent cursor could pin the delete queue's block history across commits (memory retention, not correctness). Fixed by having each EpochTask carry a fresh delete_queue.cursor(), created at the same point in prepare_commit() where the old code respawned workers — restoring the former semantics exactly.
prepare_commit() used to join every indexing worker thread and spawn
replacements on each commit. The join + thread spawn is a fixed cost of
a few milliseconds per commit, which dominates commit latency for
near-real-time writers that commit small batches at a high rate (e.g.
an in-RAM overlay index committing every few milliseconds); in profiles
of such a workload, half the committing thread's time was spent in
Thread::join.
Workers are now persistent: they live for the IndexWriter's whole life,
and a commit is an epoch barrier instead of a thread lifecycle event.
Each worker owns a channel of EpochTask { receiver, flushed }.
prepare_commit() swaps in a fresh operation channel (closing the
current one), waits on every worker's "flushed" ack (a FutureResult,
the same primitive the segment updater already uses for scheduling),
then hands each worker the next epoch's receiver.
Behavior is preserved:
- a worker's indexing error is sent through its ack and returned from
prepare_commit() verbatim, as the former join-based code did;
- a worker panic drops the ack sender, which FutureResult reports as an
error (and the existing bomb still kills the writer), so the
committer errors instead of hanging;
- Drop, wait_merging_threads() and rollback() drop the epoch channels,
so parked workers exit and joins complete as before.
IndexWriterStatus now swaps the receiver in place (replace_receiver)
instead of being recreated per commit, so the bombs held by the
persistent workers stay armed across commits.
35f073f to
039e02e
Compare
Motivation
prepare_commit() used to join every indexing worker thread and spawn
replacements on each commit. The join + thread spawn is a fixed cost of
a few milliseconds per commit, which dominates commit latency for
near-real-time writers that commit small batches at a high rate (e.g.
an in-RAM overlay index committing every hundred milliseconds); in profiles
of such a workload, half the committing thread's time was spent in
Thread::join.
Workers are now persistent: they live for the IndexWriter's whole life,
and a commit is an epoch barrier instead of a thread lifecycle event.
Each worker owns a channel of EpochTask { receiver, flushed }.
prepare_commit() swaps in a fresh operation channel (closing the
current one), waits on every worker's "flushed" ack (a FutureResult,
the same primitive the segment updater already uses for scheduling),
then hands each worker the next epoch's receiver.
Behavior is preserved:
prepare_commit() verbatim, as the former join-based code did;
error (and the existing bomb still kills the writer), so the
committer errors instead of hanging;
so parked workers exit and joins complete as before.
IndexWriterStatus now swaps the receiver in place (replace_receiver)
instead of being recreated per commit, so the bombs held by the
persistent workers stay armed across commits.