fix(search): wait for merge threads before dropping the bulk writer#6731
fix(search): wait for merge threads before dropping the bulk writer#6731Azur1337 wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6731 +/- ##
==========================================
- Coverage 89.92% 89.91% -0.01%
==========================================
Files 398 398
Lines 111157 111187 +30
Branches 111157 111187 +30
==========================================
+ Hits 99954 99970 +16
- Misses 7400 7407 +7
- Partials 3803 3810 +7 ☔ View full report in Codecov by Harness. |
5bb7102 to
6cb8ec9
Compare
Signed-off-by: Azur <azur99@web.de>
6cb8ec9 to
fa5f4d5
Compare
Hywan
left a comment
There was a problem hiding this comment.
Thanks for having worked on this, it looks good to me. Can you add a test please?
…riter Signed-off-by: Azur <azur99@web.de>
| } | ||
|
|
||
| #[test] | ||
| fn test_bulk_execute_indexes_all_events() -> Result<(), Box<dyn Error>> { |
There was a problem hiding this comment.
I've run this test with and without your previous commit and nothing changes. I quote your description of the problem:
RoomIndex::bulk_executeoepns a tantivyIndexWriter, commits, and then lets it drop without callingwait_merging_threads(). The writers background merge threads can outlive it and still be running when the nextbulk_executeopens a new writer on the same index, so the two race over the same segments
So I understand we need at least 2 bulk_execute calls. We have a single one here.
RoomIndex::bulk_executeoepns a tantivyIndexWriter, commits, and then lets it drop without callingwait_merging_threads(). The writers background merge threads can outlive it and still be running when the nextbulk_executeopens a new writer on the same index, so the two race over the same segmentsDuring bulk indexing (e.g. backfilling a room history) this surfaces as
warnings and leaves the index more fragmented than necessary
To fix this, we wait for the writers merge threads to finish before it is dropped at the end of
bulk_execute. No public api change