Skip to content

tantivy: backfill today's files once they settle - #433

Merged
tonyalaribe merged 1 commit into
masterfrom
perf/backfill-stable-today
Oct 25, 2026
Merged

tonyalaribe merged 1 commit into
masterfrom
perf/backfill-stable-today

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

After #431 (carry cap 2) the warm 7-day errors list on 6297304f still searched 273 indexes: today's 36 live files carry 264 entries (one file 37), because the cap only acts when a wave rewrites a file and these small files are not being repacked. #430's converged rule never fires for them (all far below the 128 MiB target).

The backfill now also keeps today's uncovered files whose Delta modification_time is older than 30 minutes, rebuilding each as one index within the existing per-pass budget.

Not covered by a new test: faking a Delta add's modification time is impractical; the filtering itself is the existing drop_hot_partition case table. Verified on prod via tantivy_backfill_started planned>0 and per-file entry counts.

Local signoff: make ci-signoff fmt/test/pg-smoke/e2e/clippy all pass (attested).

🤖 Generated with Claude Code

Today's partition for one project is 36 small files that the packer leaves alone,
so neither the carry cap (which acts on rewrites) nor the converged-size rule
consolidates them; one file stayed covered by 37 carried entries. A file of today
unrewritten for 30 minutes is now rebuilt as one index like a converged file.
@tonyalaribe
tonyalaribe merged commit 35ce69d into master Oct 25, 2026
13 checks passed
@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

Review of #433

The change is small and targeted, and the reasoning matches the drop_hot_partition contract. A few points:

  1. Cost on every pass. stable collects an owned String for every file in the table's snapshot that is older than 30 minutes. That is nearly all files, across all dates, and it runs for every table on every backfill pass. It's also built when skip_today is None, where it is never used. Only today's files matter to converged. Build the set lazily, or only when skip_today.is_some(), or filter to today's partition. That avoids allocating thousands of strings under the read lock.

  2. Second snapshot. stable reads table_ref separately from group_uncovered_files_by_project. A commit between the two calls could make the sets disagree. A file missing from stable is just skipped this pass, so this is safe but slightly wasteful. Taking one snapshot and reusing it for both would be cleaner.

  3. Race with the packer. The comment says a file the packer has left alone for 30 minutes "rarely" gets repacked. If it is repacked anyway, the index is voided and the work is wasted. That is bounded by the per-pass budget, so it's acceptable. It would help to expose a counter for indexes built on 'stable' files that were later invalidated, to verify the assumption in prod. Also confirm that modification_time is not bumped by DV/dedup re-adds. If it is, such files simply stay un-indexed longer, which is fine.

  4. Key matching. f.path() is a Delta-relative path and parquet_rel_of_uri must yield the same form, including any percent-encoding or partition-path escaping. A mismatch would silently make stable match nothing, which is the failure mode this PR is meant to fix. A small unit test for that equivalence (no Delta add faking needed) would be cheap and would cover the one new untested seam.

  5. Style nits. TODAY_STABLE_MILLIS is inserted between a doc comment and its const, so check that GC_PHASE_BUDGET's grouping with the next const is still tidy. The test-coverage note in the description is reasonable. Making converged testable by passing in a stable set would let the existing case table exercise it.

No correctness blockers; (1) and (4) are worth addressing before merge.

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