feat: Improve on-disk cache init - #87
Conversation
…read Signed-off-by: Darkheir <raphael.cohen@sekoia.io>
Signed-off-by: Darkheir <raphael.cohen@sekoia.io>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized, add targeted tests for the new behavior, and only minor robustness improvements were identified.
Pull request overview
This PR improves the DiskSizedCache startup path by parallelizing shard scanning to rebuild the LRU index faster, and it hardens cache accounting against a race where a failed disk read could incorrectly drop a newly re-inserted entry.
Changes:
- Parallelize on-disk shard scanning during cache open, then globally sort by mtime to restore cross-shard recency.
- Introduce per-entry “generation” tracking so missing-file cleanup won’t remove a concurrently replaced entry.
- Add tests covering recency restoration across shards and ensuring vanished entries are removed from the index/capacity accounting.
File summaries
| File | Description |
|---|---|
| quickwit/quickwit-storage/src/cache/disk_sized_cache.rs | Adds parallel shard scanning + generation-based index bookkeeping to speed open and avoid races when files disappear/are replaced. |
Review details
Suppressed comments (1)
quickwit/quickwit-storage/src/cache/disk_sized_cache.rs:466
entries.into_inner().unwrap()will also panic if the mutex is poisoned. If the intent is to keep cache opening best-effort even under partial failures, it’s safer to recover the inner Vec on poison.
entries.into_inner().unwrap()
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let shard_entries = scan_shard(shard_path); | ||
| entries.lock().unwrap().extend(shard_entries); | ||
| } |
There was a problem hiding this comment.
I don' think the lock can be poisoned here.
| .min(shard_paths.len()); | ||
| let next_shard_idx = AtomicUsize::new(0); | ||
| let entries = Mutex::new(Vec::new()); | ||
| std::thread::scope(|scope| { |
There was a problem hiding this comment.
nit, I think using a rayon threadpool you make this a tiny bit easier to proofread
| const SCAN_THREADS_PER_CPU: usize = 4; | ||
|
|
||
| /// Upper bound on the number of shard-scanning threads. | ||
| const MAX_SCAN_THREADS: usize = 128; |
There was a problem hiding this comment.
should we maybe apply this parallelism limit to all blocking (spawn_blocking) tasks and use one thread pool for the entire disk sized cache?
I don't think so, this is already fairly complicated 😉 |
Description
There is one last potential optimization that would help the searcher start faster:
Make the init return immediately (before the cache has been warmed up) and always check the disk on cache miss during this warm up phase.
Do you think the added complexity is worth it ?
How was this PR tested?
Added test