feat(partitions): implement state transfer - #3808
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3808 +/- ##
============================================
- Coverage 76.40% 75.58% -0.83%
Complexity 1046 1046
============================================
Files 1334 1337 +3
Lines 165228 168037 +2809
Branches 137583 140469 +2886
============================================
+ Hits 126242 127007 +765
- Misses 35271 37175 +1904
- Partials 3715 3855 +140
🚀 New features to boost your workflow:
|
Removing the namespace-exemption import left its #[cfg(debug_assertions)] attribute orphaned onto the unconditional server_common import below it, so every release build of the shard crate fails with 112 unresolved-name errors. Debug builds keep the import and CI's -D warnings lane rides clippy, which builds debug, so nothing upstream could observe it. Co-Authored-By: Claude Fable 5 <[email protected]>
numinnex
changed the title
github-actions
Bot
added
S-waiting-on-author
|
/ready |
github-actions
Bot
added
S-waiting-on-review
github-actions
Bot
added
S-waiting-on-author
|
/ready |
github-actions
Bot
added
S-waiting-on-review
5d588ff to
9494dd6
Compare
|
/ready |
github-actions
Bot
added
S-waiting-on-review
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
the offset/op frontier is claimed in four places that disagree about what the claim means (boot re-derivation, the install fence, the sequencer after clear_all, the unlink window). since replicas re-stamp base_offset from their local counter, a wrong claim becomes divergent persisted bytes rather than an inconvenience. the partition superblock already spends a field on a commit_max its own comment calls a dead write - giving the frontier a durable home there closes most of the family at once.
two decisions sit above the individual comments: the StateTransferTargetHeader mid-struct layout change in the published binary_protocol crate needs an explicit call (tail-append plus a real version signal if mixed-version links should ever work)
github-actions
Bot
added
S-waiting-on-author
|
/ready |
github-actions
Bot
added
S-waiting-on-review
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
shard/lib.rs:5046 (pre-existing): a manifest whose first entry isn't METADATA_SNAPSHOT - unknown kind, or a checksum mismatch ahead of it - leaves generation = None, so :5116 short-circuits, burn_decode_failure never charges, and :5138 re-arms the same peer: an unbounded full-manifest re-pull loop. it needs a hostile or buggy peer today (partition kinds route to the partition arm, which refuses unknown kinds and backs off), but the fix is cheap - the accept-time generation at :4588-4591 is computed and then discarded; store it on the session and the charge becomes unconditional.
| // would pass the serve gate and offer that emptiness onward, making a peer | ||
| // unlink its own chain. Leaving it `None` costs one spurious full | ||
| // re-transfer on the legitimate empty-install restart; a false caught-up | ||
| // claim is not recoverable. A durable home for the frontier (the partition |
There was a problem hiding this comment.
this comment is right, and it's the blocker: the frontier's only durable carrier is a segment file name. three routes leave the counter at 0 while the group is at N - the unsynced empty-install dirent (state_transfer.rs:1966), a crash in the swap window (state_transfer.rs:1875), and the ConvergeFailed fence-and-rebuild path which needs no crash at all. after any of them, repaired_window_is_offsets_only accepts a floor with no batch anchor and the next append stamps base_offset 0 and re-derives batch_checksum (send_messages2.rs:819-822) - this replica durably persists different bytes than its group for the same committed op, silently. the anti-rewind check can't catch it either, it's gated on local_next_offset > 0.
stamping the frontier into the partition superblock and re-seeding at boot closes all three. two caveats: commit_max is an op number (vsr_state.rs:60-66), so this needs either a new VsrState field (decode demands exactly ENCODED_LEN bytes, so that's a version bump) or a documented overload; and persist_superblock_if_needed runs per prepare but needs_superblock_persist only fires on view/log_view change (impls.rs:1535-1542), so the trigger needs extending too - a frontier field alone would only persist on view changes.
| // artifact's frontier, exactly where rotation would have put | ||
| // it, so post-install traffic lands in a segment named for the | ||
| // offsets it holds. | ||
| self.install_empty_segment(config, offsets_wire.next_offset) |
There was a problem hiding this comment.
install_empty_segment passes file_exists = false, which skips both writer creation fsyncs (segment_storage/mod.rs:111), and no fsync_dir follows - the per-log one at :1910 is inside the staged loop, which is empty on this path. the frontier-bearing .log dirent is page-cache only; crash after the install and boot walks an empty dir and re-derives the counter at 0.
| // fewer fsyncs than this: one-per-pair would lean on intra-directory | ||
| // rename ordering POSIX does not grant. | ||
| // | ||
| // KNOWN WINDOW, above and here: the old chain's unlinks are already |
There was a problem hiding this comment.
crash inside this window and the frontier is unrecoverable, not just unsynced: the old chain is already unlinked and fsynced, the staged logs aren't renamed yet, and the boot sweep unconditionally deletes .log.staging (segment_recovery.rs:270-273) - so the only durable copy of the frontier gets deleted at the next boot.
| topic_id, | ||
| partition_id, | ||
| )); | ||
| partition.set_partition_dir(partition_dir); |
There was a problem hiding this comment.
the zeroing right below is reachable with no crash at all: failed install then failed converge raises ConvergeFailed, the shard fences and tombstones, and the reconciler rebuilds through here at offset 0. on a multi-replica group that's a quorum-invisible backup at counter 0; on a single-replica group restarted stays false (replica_count > 1 is part of that check), so it takes consensus.init() and claims view-0 primaryship at counter 0.
|
|
||
| // A receiver that missed a purge must not be re-wiped by the | ||
| // reconciler right after installing post-purge data. | ||
| self.applied_purge_generation = self |
There was a problem hiding this comment.
the max() has no staleness refusal anywhere in the check phase - the only generation comparison is purge_advances at :1726 and it only widens permission. a peer that hasn't applied the purge yet offers generation N-1 with pre-purge segments; install succeeds, applied stays N, and the reconciler's committed > applied gate is N > N - it never re-wipes. purged data is durably resurrected and survives until this replica restarts (applied_purge_generation is memory-only, so a restart resets it to 0 and the boot re-purge fires). the rewind check can't substitute: post-purge local_next_offset is 0 which kills the :1732 conjunct, and once there's post-purge data the stale offer's next_offset is larger, not smaller. the comment at iggy_partition.rs:3413 closed the in-flight case but this pre-armed one is open, and the test at iggy_partition.rs:5541 only covers the advancing direction.
a fix that works without persisting the generation: refuse a regressing generation at the shard call site (lib.rs:6408), comparing against the durable committed generation from the metadata plane - not this partition's applied value, which is 0 after every restart - and route it through the existing Err arm at lib.rs:6464, which already rotates the peer. worst case is one wasted pull against a peer that hasn't caught up on the purge yet.
| Ok(entries) => entries | ||
| .flatten() | ||
| .map(|entry| entry.path()) | ||
| .filter(|path| { |
There was a problem hiding this comment.
same suffix predicate as :1088 (only the closure binding differs). if these get unified, parameterize the predicate and leave sweep_staging_except's staging-only filter alone - the keep-lists at :1664 and :1808 hold only staging paths (and purge passes &[]), so widening the sweep predicate to the three-suffix list would unlink every live .log/.index on the partition, worst at the :1665 sweep which runs at descriptor-accept on a serving partition. the segment_recovery walk is a stateful fold, not the same shape - leave it out.
| path: index_staging.clone(), | ||
| source, | ||
| })?; | ||
| Ok(StagedSegmentMeta { |
There was a problem hiding this comment.
field-for-field identical to the literal at :1525-1535 - worth one shared constructor.
| "re-answering a partition state transfer request from the offer already served" | ||
| ); | ||
| let (serving_view, serving_commit_max) = serving_progress(partition); | ||
| self.send_state_transfer_target( |
There was a problem hiding this comment.
three copies of serving_progress + send_state_transfer_target in this fn (cached here, fresh at :5548, and the Err arm at :5588) differing only in the descriptor. resolving the descriptor in one match with a single send site drops ~25-30 lines.
| source, | ||
| })?; | ||
| memo.hashed_len = size; | ||
| memo.checksum = memo.hasher.finish(); |
There was a problem hiding this comment.
SegmentChecksumMemo.checksum is a second source of truth - StateArtifactHasher::finish takes &self, so both reads can use memo.hasher.finish() and the field plus its two assignments can go. the checksum: 0 initializer is already quietly wrong (xxhash3 of empty input isn't 0) - inert today, but exactly the drift a duplicate invites. dropping the memo on the failure path stays safe since it's removed from the map before hashing.
| } | ||
|
|
||
| impl ArtifactProgress { | ||
| #[must_use] |
There was a problem hiding this comment.
this inherent complete shadows the ChunkProgress default at :82 with a semantically identical body, and call sites split silently by genericity - generic paths (next_pending_chunk, append_chunk) resolve to the trait method, concrete ones (shard/lib.rs:6309 and the test) to the inherent. no const callers exist, so delete the inherent; otherwise any future trait-side override silently misses the concrete sites.
github-actions
Bot
added
S-waiting-on-author
3 participants
Implement the
StateTransferprotocol for partition plane. Additionally use thesuperblockmechanism introduced in #3767 inside of the partition plane.