Repository navigation
fix(server): detach demand publish from the flush and merge-request path - #351
Merged
beinan merged 1 commit intoOct 8, 2026
Merged
Conversation
lance-format#345 awaited the shard manifest read and the etcd put inline in route_merge, before the merge request. A hung publish consumed the flush pass timeout, skipped that table's merge request, and delayed every later table in the same pass - up to 150 s per table with default timeouts. That violates the P1 rule that telemetry never affects existing scheduling. The publish is now spawned, never awaited: bounded by a 5 s timeout and a process-wide semaphore of 4 in-flight publishes. When the slots are full the publish is dropped and counted; the next flush republishes an absolute watermark, so nothing is lost. Every outcome lands on rollout_wal_demand_events_total{result = written | unchanged | no_manifest | failed | timeout | dropped}. Sweepable gains Clone as a supertrait (all implementors are Arc<RwLock<_>>). Validation: new etcd test with a store whose watermark read hangs forever asserts the flush pass returns in under 2 s, the merge request is still issued, and eight further passes while the publish is still hung also return immediately. The earlier publish test now polls for the detached write with a 10 s bound. All 9 sweeper tests pass; clippy -D warnings, fmt, typos clean. Refs lance-format#333. Review finding 1 on lance-format#345. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
17 of 18 tasks
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
1 participant
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.
Part of #333. Fixes review finding 1 [P1] on #345 (credit: production review).
Problem
#345 awaited the manifest read + etcd put inline in
route_merge, before the merge request. A hung publish ate the flush pass timeout, skipped that table's merge request, and delayed every later table in the pass — up to 150 s/table with defaults. Telemetry was on the scheduling path, which P1 forbids.Fix
Publish is spawned and never awaited: 5 s timeout, process-wide semaphore of 4 in-flight publishes, drop-and-count when full (the next flush republishes an absolute watermark, so nothing is lost). All outcomes on
rollout_wal_demand_events_total{result=written|unchanged|no_manifest|failed|timeout|dropped}.Sweepable: Clone(all implementors areArc<RwLock<_>>).Validation
hung_demand_publish_never_delays_flush_or_merge_request: a store whose watermark read ispending()forever → flush pass returns in < 2 s, merge request still issued, 8 further passes while still hung return in < 2 s total.flush_publishes_shard_demand_…adapted to poll for the detached write (10 s bound).-D warnings, fmt, typos clean.🤖 Generated with Claude Code