fix(server): the count-triggered self-merge takes the worker's merge slot - #289
Merged
Merged
Conversation
…slot ROLLOUT_MERGE_AFTER_GENERATIONS merges a shard as soon as it has N pending generations, on the 30 s flush sweeper. That merge bypassed the per-worker merge slot (ROLLOUT_MERGE_CONCURRENCY), so turning the count trigger on would stack sweeper merges on top of the master's requests with no bound. It now acquires the same slot, so the bound holds no matter who starts the merge. Needed to enable pending-driven merging in production: hot stores get merged when they accumulate generations, not when the master's 600 s sweep comes around, which is what keeps reads over them fast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review on lance-format#289: awaiting the slot before merge_if_due stalled the flush of every store behind the first one whenever all slots were busy, even with the count trigger off. The pass now try_acquires; when the slots are full the self-merge is skipped (rollout_wal_self_merge_skipped_total) and flushing continues. Regression tests: two stores flush with all slots held and the trigger off; a store merges when a slot is free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Addressed: the pass now |
beinan
added a commit
that referenced
this pull request
Oct 2, 2026
…292) A stalled WAL merge can hold a master task indefinitely. Retrying after only an HTTP timeout can overlap old and new base-table writes. This change owns each merge independently of its HTTP connection, closes commit admission on recovery, and fences every admitted manifest version before handing the table to another executor. Worker fan-out remains serial. - **Default-off, per-table rollout:** explicit `MERGE_OWNED_TARGETS` and `MERGE_DRAIN_TARGETS`, empty by default. Deploy capable binaries first, drain only the selected table, then enable it. An owned request never falls back to legacy HTTP. Drain reconciles existing owned work while preventing new maintenance mutations on that target; other tables retain their job pools. - **Keep pending/timer triggers:** #289 configuration remains valid. For enabled rollout/generic tables the worker checks its own manifest and posts coalesced demand; the master picks it up every 15 seconds through normal dedupe, serial ownership, and worker slot/byte limits. Unselected tables retain direct self-merge. Scheduling latency for enabled tables needs measurement. - **Separate time budgets:** slot queue defaults to 600 seconds; execution starts after slot acquisition and has a configurable 3600-second ceiling. A separate 600-second no-progress limit observes completed batch/phase/storage work. Repeated heartbeats do not extend it. No hard-coded 600-second execution cap remains. Timeouts cancel executions, not pods, and handoff still requires completed writes or proven storage fences. - **Worker etcd connection is lazy:** outage does not fail worker startup or disable ingestion/flush. Owned admission and further commit authorization fail closed; already-authorized writes may finish. - **Bound repeated failures:** durable per-target/endpoint budgets survive task recreation and worker demand; three initial transient attempts, then backoff, hourly probes and attention after 15 failures. Healthy shards remain eligible; partial failures stay visible. Missing data never triggers destructive merge repair. The read-only failure inspection API is split into #293 (118 lines across the API, its pagination regression, and documentation). Validation for the final runtime changes: master **88**, worker **92**, and shared protocol **13** tests passed, including isolated-etcd cases for slot waiting, no-progress cancellation, HTTP disconnects, orphan fencing, drain recovery probes, and retained retry budgets. Clippy with warnings denied and formatting passed on the final source. Actual delayed-commit/new-WAL/blob recovery tests passed. The compaction/merge concurrency test now permits only Lance's typed `RetryableCommitConflict` from index preparation, retries once after compaction completes, and verifies the physical row count plus exact IDs; the corrected test passed once plus **10/10** repeated runs. The broad core run completed: **258 passed / 3 existing benchmarks ignored**, with only the concurrency assumption described above failing; that corrected test was then validated in the 11 runs above. CI is running on `c0c0b5a`. **Draft: staging soak is still required.** Legacy untracked writes cannot be automatically fenced, and changing flags is not proof that they drained. All replicas/helpers must follow the per-table migration and rollback sequence in `docs/merge-recovery.md`. Progress inside opaque Lance operations is coarse: measure realistic large-blob phase latency, queue time, memory/OOM, etcd overhead and p95/p99 before enabling production targets. No production deployment or pod/configuration changes are part of this PR. --------- Co-authored-by: Beinan Wang <>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ROLLOUT_MERGE_AFTER_GENERATIONSis the pending-driven merge trigger: when a worker's own shard has ≥ N flushed generations, the 30 s flush sweeper merges it. That is exactly what hot stores need (a store written every few seconds regrows hundreds of generations between the master's 600 s sweeps, and every read has to open all of them — 8–13 s per read on a 176-generation store today).But that sweeper merge bypassed the per-worker merge slot (
ROLLOUT_MERGE_CONCURRENCY, #278). Enabling the count trigger would stack sweeper merges on top of the master's merge requests with no memory bound.Fix
flush_passtakes the worker's merge-slot semaphore and acquires a permit aroundmerge_if_due, so sweeper-initiated and master-initiated merges share one bound. No behaviour change while the count trigger is0(the default and current production value).Verification
Server suite passes (85); clippy clean. Production enablement is a config change in the deployment repo (
ROLLOUT_MERGE_AFTER_GENERATIONS=64), validated on staging first.🤖 Generated with Claude Code