Skip to content

tantivy: rebuild a rewrite's index past 2 carried entries, not 8 - #432

Merged
tonyalaribe merged 1 commit into
masterfrom
perf/carry-cap-2
Oct 25, 2026
Merged

tonyalaribe merged 1 commit into
masterfrom
perf/carry-cap-2

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

Follow-up to #427/#430. Today's partition for 6297304f is 33 live files, all far below the 128 MiB packing target, so the converged-file backfill (#430) never applies; at a carry cap of 8 those 33 files carried 260 index entries (none stale), and a warm 7-day errors list spends ~2.8 s of its 3.8 s searching them.

MAX_CARRIED_ENTRIES 8 → 2. Hot-tail wave outputs are small, so the extra rebuilds are cheap (~1 core estimated at ~2.4 waves/min against 48 cores, 2-7 in use).

Test: carry_forward_declines_past_the_carried_entry_cap now 3 inputs decline / 2 carry; red with the cap at 3.

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

🤖 Generated with Claude Code

Today's partition stays below the packing target, so the backfill never
consolidates it, and at a cap of 8 the 33 live files of one project were
searched as 260 indexes (~2.8 s of a 3.8 s warm 7-day list). Hot-tail outputs are
small, so rebuilding them is cheap.
@monoscope-bot

monoscope-bot Bot commented Oct 25, 2026

Copy link
Copy Markdown

Monoscope production impact · Worth checking

Project 87576849-4941-49d3-a15d-680fef88a1a8 · reviewed 93f7b39089943c5c24a307093b247bc1d771eb83 · advisory

Evidence window (UTC): 2026-10-18 18:37:20.411825172 UTC → 2026-10-25 18:37:20.411825172 UTC

src/tantivy/search.rs:234 (Head)

Production details are available in Monoscope.

telemetry evidence

Coverage: Open Monoscope for evidence and coverage details.

Review history and settings

@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

Review: small, focused change (const 8 → 2 plus a matching test update). Looks good to me.

  • Correctness: The test boundary is exact: 3 inputs decline and 2 carry, so it fails at the old cap. The PR says it was checked red with the cap at 3.
  • Tradeoff: A lower cap means more index rebuilds on the write and maintenance path. The ~1 core estimate is plausible against 48 cores. The PR doesn't name a timefusion_stats metric to watch after deploy, which the repo guidance asks for. I'd watch index-build or backfill counters, and maintenance wave latency, for a couple of hours once it's live.
  • Rollback: This is a hardcoded const with no kill switch. The cap is cheap to revert, but an env knob would make a bad prod outcome a config change instead of a deploy. This is optional, and I wouldn't add it if you want to keep the change minimal.
  • Doc comment: It is fairly long and mixes incident data (33 files, 260 indexes) into a code comment. A one-line rationale plus a pointer to the PR would match the repo's minimal-comment style.

No blocking issues.

🤖 Generated with Claude Code

@tonyalaribe
tonyalaribe merged commit 93f7b39 into master Oct 25, 2026
13 checks passed
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