MB-72725: Improve efficiency of index compaction#2373
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Scorch index compaction planning/execution by making merge decisions account for logical deletions (via “live” sizing), avoiding degenerate single-segment plans that don’t reclaim deletes, and introducing merged segments in a single introducer push to reduce contention and improve concurrency.
Changes:
- Add “live file size” estimation and use it in merge planning (budgeting + vector eligibility) so heavily-deleted segments can become eligible for compaction.
- Adjust merge planning/roster selection to avoid non-converging “solo fully-live” merge tasks and to ensure only meaningful merge tasks count against the budget.
- Overhaul/add unit tests (Scorch + merge planner) to assert convergence and expected final segment layouts; update docs/examples for config keys.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| index/scorch/snapshot_segment.go | Adds LiveFileSize() estimation to SegmentSnapshot. |
| index/scorch/scorch_test.go | Expands persister/merge tests and adds a deletion-vs-merge-introduction regression test. |
| index/scorch/scorch_knn_test.go | Updates KNN persistence test to use refcounted snapshot + configured persister options. |
| index/scorch/rollback_test.go | Fixes test name typo (Rollback). |
| index/scorch/mergeplan/merge_plan.go | Updates planner to use “live” sizing, adds roster selection tweaks, and introduces budget currency handling. |
| index/scorch/mergeplan/merge_plan_test.go | Refactors tests to run multi-cycle convergence assertions and adds new planner coverage. |
| index/scorch/merge.go | Changes merge execution to introduce all merged segments in one shot; updates bookkeeping and history mapping. |
| index/scorch/introducer.go | Updates merge introduction logic to use the new batch-index field when applying deletes. |
| docs/persister.md | Updates configuration examples to match expected JSON key casing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case <-s.closeCh: | ||
| err = segment.ErrClosed | ||
| return err | ||
| case s.merges <- sm: |
There was a problem hiding this comment.
from what i understand, we would now wait for all the file merges (which can be expensive and time consuming) to complete and then all of them are introduced in a single step.
however, all this while the root would end up having potentially larger number of file segments, so the snapshot with leaner segments wouldn't be available for search.
also, the persister would be less likely to pick up work and reduce the memory pressure during this window since the root wouldn't change for a long duration.
so, wouldn't it be better to introduce the segments one-by-one instead to move the system forward more frequently?
There was a problem hiding this comment.
- Assuming we have
Ntasks, andTis the total time it takes to just perform the merge+open operation;Twould be a constant irrespective of the scenario chosen. - When there is active ingest, the root will constantly be updated by the introducer so the persister would always be picking up work, and the reduction in memory pressure would be driven by ingest and not the merger.
- In the case of no ingest however, the persister would wait for a new snapshot to be swapped into root with all
Nmerged segments (fromNtasks); this is arguably a better scenario as earlier we would have persisted N different epochs all representing the same data.
- In the case of no ingest however, the persister would wait for a new snapshot to be swapped into root with all
- If we consider the time taken to introduce a merge as
I, thisIwill be a constant as its a fixed workload of scanning the root while holding the merged segment history. Hence the overall time taken for 1 merge cycle to complete will beT + Iin case of batch introduction andT + NIin case of per-task introduction. TheNfactor here would cause issues as mentioned in the next point.- Earlier we were having 1 persister worker and doing one-shot merge and persist operation, this would mean that we could have
Xnumber of files in hand in the snapshot taken by the merger. GivenNumPersisterWorkers(P) +MaxSizeInMemoryMergePerWorkercombination we could not be persisting at leastP*Xnumber of files onto disk. - This would cause a snowball effect, where the merger starts off with small
Nand progressively increasesNdue to many tasks being generated by the planner due toP*Xnumber of files. This is always true irrespective of whether we use batch introduction or task introduction. But theNIterm could become significant in the per-task introduction case.
- Earlier we were having 1 persister worker and doing one-shot merge and persist operation, this would mean that we could have
- The transient window between the time at which
task 1lands into the root and the time at whichtask Nlands into the root in the per-task-introduction scenario would cause some degree in loss of latency, but i was thinking of offsetting this cost by introducing a concept callednumMergeWorkersin a later PR. - Since the segments are disk-bound and mmap'd, we could potentially perform each merge task in parallel, and using the semaphore approach documented in the previous PR, we could amortize the cost of
Nmerge tasks into1 merge task- thereby avoiding the transient loss of latency. - The "numMergeWorkers" could be enabled for FTS only workloads and fast-merge enabled vector workloads for example. This would directly counterbalance the snowball-effect mentioned earlier as well.
Uh oh!
There was an error while loading. Please reload this page.