Skip to content

refactor(storage): download manager — collapse the async onion on the search read path#6580

Draft
PSeitz-dd wants to merge 2 commits into
mainfrom
download_manager
Draft

refactor(storage): download manager — collapse the async onion on the search read path#6580
PSeitz-dd wants to merge 2 commits into
mainfrom
download_manager

Conversation

@PSeitz-dd

Copy link
Copy Markdown
Contributor

What

Collapses the S3 search read path from a deep stack of #[async_trait] impl Storage decorators + tantivy Directory wrappers into a synchronous decision core plus one thin async download layer, and unifies caching in quickwit-storage.

Historically, fetching a byte range of a split traversed timeout/retry → fast-field cache → on-disk split cache → request debouncing → bundle-offset translation → ephemeral byte-range cache → static hotcache. Every layer allocated a boxed future and added an .await, even though only the actual network/disk read performs I/O.

How

New SplitDownloadView in quickwit-storage/src/download_manager/:

  • Synchronous coreresolve() performs every cache lookup, the bundle logical→physical offset translation, and dedup keying without allocating a single future. Returns Resolution::Hit(OwnedBytes) (zero futures, immediate) or Resolution::Miss(DownloadPlan).
  • One thin async layerdownload(plan) performs the actual read (on-disk split-cache fill or object-store GET), bounded by the backend's existing semaphore, debounced and retried, then writes the result back into the caches synchronously on completion.

Caching is unified and keyed by the physical split path joined with the logical file name inside the split, plus the file-relative byte range — keeping the long-term .fast field cache and the ephemeral byte-range cache cleanly differentiated per file while remaining globally unique (split files are ULID-named).

Removed / folded

  • timeout_and_retry_storage.rs (−279) — folded into the single download() layer
  • cache/quickwit_cache.rs (−207) — unified into the download manager
  • directories/storage_directory.rs (−142) — replaced by download_manager_directory.rs
  • debouncer.rs slimmed (−118); the Storage trait is retained unchanged

Profiling (prod A/B on cloudprem-ws-pascal-searcher*)

Compared a download-manager build against the previous build under comparable load (~4.1–4.4 cores).

  • The onion is structurally gone in the flame graphs: the read path is now DownloadManagerFileHandle::read_bytes_async → download_manager::download::fetch_slice → S3, versus the old 6-layer HotDirectory → CachingDirectory → StorageDirectory → StorageWithCache → DebouncedStorage → S3 chain. Debouncing is preserved (moved inside the manager).
  • CPU is neutral — the storage subtree is ~14% in both; dominant cost is unchanged real work (tantivy aggregation, S3 TLS/recv, byte-copy). The onion's cost was per-call allocations and complexity, not CPU cycles.
  • Caveat: allocation profiling is disabled for this service, so the boxed-future allocation reduction (the primary thesis) can't be quantified from these profiles.

Notes

  • analysis.md (design doc + decisions) is included in this branch for review context; can be dropped before merge if undesired.
  • Storage trait is intentionally kept — no new ObjectDownload trait.

Checklist

  • cargo clippy --workspace --all-features --tests
  • cargo +nightly fmt --all -- --check
  • cargo nextest run --all-features
  • license headers / log format / typos / machete

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants