fix(server-ng): stop redundant partition rebuilds resetting offsets - #3846
fix(server-ng): stop redundant partition rebuilds resetting offsets#3846numinnex wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3846 +/- ##
=============================================
- Coverage 76.58% 35.04% -41.54%
Complexity 1046 1046
=============================================
Files 1347 1345 -2
Lines 171018 148900 -22118
Branches 142372 120330 -22042
=============================================
- Hits 130967 52178 -78789
- Misses 36233 95359 +59126
+ Partials 3818 1363 -2455
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
fix itself checks out. one blocker in the comments: the publish half has no test that can fail.
two pre-existing issues found while tracing, filing separately:
- bootstrap.rs:1794: the failed-quarantine tombstone doesn't fence the reconciler (
containsreads onlynamespace_map), so the namespace gets rebuilt - truncating the refused chain's segment 0 when the superblock frontier is 0 - and lands still-tombstoned with nountombstonepath: unreachable forever, fast-skip pinned off.fence_partition_for_rebuild's failure arm hits the same wedge at runtime.
| Some(0), | ||
| "the discarded op must not repoint the routing row" | ||
| ); | ||
| assert_eq!( |
There was a problem hiding this comment.
stays green even with all three publish_current_offset() calls deleted - every build here adopts at offset 0 and the hand set_current_offset(3) lands after the pass. nothing else covers the publish half (vsr-off CI spawns the legacy binary). fix: partition.offset.store(N, Ordering::Release) before enqueuing InsertOwned, then assert stats.current_offset() == N - must fail when the publish is removed.
| leaking its VSR group and segment writers" | ||
| ); | ||
| assert_eq!( | ||
| shard.shards_table().shard_for(ns), |
There was a problem hiding this comment.
can't fail - single shard, adopt would write ShardId::new(0) too. capture epoch_before after the first pass, stage the op with epoch_before + 1, assert epoch_for(ns) == epoch_before.
| // An earlier pass already built this one and the pump has not applied it | ||
| // yet, so the `contains` test above reads false for finished work. | ||
| // Rebuilding is not a wasted-effort question: the second incarnation | ||
| // shares the namespace's `PartitionStats` with the live one and re-opens |
There was a problem hiding this comment.
nothing is live here (contains was false) - the sibling is queued for adoption. same wording in the test doc at 1632; the copy at shard/lib.rs:1922 is fine.
| // build already planted its initial segment over the live | ||
| // incarnation's path and folded that into the namespace's | ||
| // shared stats. | ||
| if partitions.contains(&namespace) { |
There was a problem hiding this comment.
this drop is silent, but per the comment above it only fires after the build truncated the live writer's file - worth tracing::error! + a metric.
| /// keys, so the row survives with the DEAD incarnation's `created_revision`. A | ||
| /// presence-only gate never refreshes it, and nothing else writes a non-owner's | ||
| /// row. | ||
| fn stage_routing_row( |
There was a problem hiding this comment.
single caller, wrapping shards_table_has_epoch (also single caller), while 593 spells the same predicate inline. inline this, keep shards_table_has_epoch and use it at 593.
| owning_shard: u16, | ||
| counters: &mut PassCounters, | ||
| ) { | ||
| if shards_table_has_epoch(ctx, ns, epoch) { |
There was a problem hiding this comment.
no mirror of the InsertOwned guard, so a lagging pump gets one duplicate InsertRouted per namespace per pass and the fast-skip stays disarmed. harmless - just worth a doc line that the asymmetry is deliberate (a queue scan here would go quadratic).
| // building instead of aging its frames. | ||
| if ctx.shard.has_staged_insert_owned(ns) { | ||
| counters.already_staged += 1; | ||
| staged.insert(ns); |
There was a problem hiding this comment.
staged duplicates what has_staged_insert_owned answers and goes stale across the reconcile_removals awaits. drop the set + staged_unapplied param and query the queue per parked namespace; parked early-returns when empty.
| /// on the live incarnation, which then rejects every | ||
| /// `store_consumer_offset` above 0 with `InvalidOffset` until the next send | ||
| /// re-seeds it. | ||
| pub fn publish_current_offset(&self) { |
There was a problem hiding this comment.
all three call sites funnel through IggyPartitions::insert - moving the publish inside makes the contract unforgettable, guards the unit-unreachable boot site through the same test, and lets this go crate-private.
github-actions
Bot
added
S-waiting-on-author
2 participants
No description provided.