Skip to content

tantivy: serve bundle ranges from the foyer disk tier - #430

Merged
tonyalaribe merged 1 commit into
masterfrom
perf/tantivy-disk-cache
Oct 25, 2026
Merged

tonyalaribe merged 1 commit into
masterfrom
perf/tantivy-disk-cache

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

Cold first loads of filtered lists (7-day errors list on 6297304f: ~8.4 s) read ~1.1 GB of bundle ranges, and the #428 counters show 100% of it is fast-field columns, which tantivy loads whole per index. The 4 GB block cache makes repeats free, but every deploy empties it.

Bundles are immutable (every publish uploads to a fresh generation path), so StoreSource now checks the shared foyer cache before a ranged GET and inserts what it fetched disk-only (SharedFoyerCache::{get_immutable, put_immutable}), so ranges survive restarts without displacing the in-memory working set. New counters: range_disk_hits, range_disk_hit_bytes; range_reads/range_read_bytes now count object-store fetches only.

Test: bundle_ranges_survive_a_restart_in_the_disk_cache (a second reader with an empty block cache makes zero object-store range reads); red with the disk cache disconnected.

Local signoff: fmt/test/pg-smoke from the first run, e2e/clippy rerun after the first run was terminated externally; ci.sh gate reports all five attested for this tree.

🤖 Generated with Claude Code

A cold 7-day errors list reads ~1.1 GB of bundle ranges, all fast-field columns
(prod counters), which tantivy loads whole per index. The 4 GB block cache makes
repeats free, but every deploy empties it and the first view of each dashboard
re-read those columns from S3. Bundles are immutable (every publish takes a fresh
generation path), so their ranges are cached disk-only under the block cache and
survive restarts.
@monoscope-bot

monoscope-bot Bot commented Oct 25, 2026

Copy link
Copy Markdown

Monoscope production impact · Coverage unknown

Project 87576849-4941-49d3-a15d-680fef88a1a8 · reviewed 596e644e3c26c6f477fee63874ae830302bf0968 · advisory

Evidence window (UTC): 2026-10-18 17:18:07.901369863 UTC → 2026-10-25 17:18:07.901369863 UTC

Coverage: Open Monoscope for evidence and coverage details.

Review history and settings

@tonyalaribe
tonyalaribe merged commit 596e644 into master Oct 25, 2026
13 checks passed
@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

Review

The change is small and well-targeted. Bundles are immutable because every publish takes a fresh generation path, so a range cache keyed on path and range is safe. The tantivy-bundle: prefix keeps these keys from colliding with object-store cache keys. The test is meaningful and the PR says it goes red with the disk cache disconnected. I haven't built or run anything locally.

Things to consider

  1. Hits aren't validated. get_immutable returns whatever is stored under the key. A data.len() != range.end - range.start check in StoreSource::read would treat a truncated or corrupt disk entry as a miss. Tantivy would otherwise get a short read and fail in a hard-to-diagnose way. It's cheap insurance, since foyer's disk tier is shared and has had its own recovery paths.

  2. Large fast-field ranges share the disk budget with Delta data. The PR says fast-field columns are ~100% of the 1.1 GB read. Those are inserted as single entries into the same foyer disk cache that holds the parquet data. Please confirm that:

    • entries larger than the foyer region or large-object limit are handled, since an oversized insert may be dropped silently or evict a lot at once;
    • a burst of cold bundle reads can't churn Delta data out of the disk tier.

    A range_disk_puts counter, or a note on measured disk-tier eviction after deploy, would make the second point observable.

  3. Counter semantics changed. range_reads and range_read_bytes now count object-store fetches only. This is documented in the PR, but any dashboard or alert that treats them as total range reads will start under-reporting. The range_read_{fast,terms,store}_bytes counters are unchanged in the diff; check whether they're still incremented on disk hits, since they're described as splits of range_read_bytes.

  4. No single-flight on a miss. Concurrent misses for the same range each hit S3 and each insert. This is pre-existing behaviour at the block-cache level, so it isn't a blocker. Cold loads after a deploy are the case where it matters most.

  5. Test. The polling loop builds a fresh reader each iteration. That is fine for correctness, but each iteration also re-reads the head from the object store. Waiting on the foyer disk flush, if there's a hook for it, would be less timing-dependent than the 10s deadline. Minor.

Style

This follows the repo's conventions: it's concise, with no new abstractions beyond the two cache methods. The with_disk_cache builder is duplicated at both construction sites (main.rs and server/mod.rs). That mirrors the existing duplication there, so it's fine.

LGTM once (1) and (2) are addressed or explicitly waved off.

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.

1 participant