krishvishal commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3957670320
##########
core/shard/src/lib.rs:
##########
@@ -5654,55 +5792,37 @@ where
return None;
}
session.idle_ticks = 0;
- Some((session.peer, session.nonce, session.to_op))
+ Some((session.peer, session.nonce, session.from_op,
session.to_op))
})
};
- if let Some((peer, nonce, to_op)) = stalled {
- // A session pins its peer and fences every arming site while it
- // stands, so a peer that cannot answer wedges the plane harder
than
- // having no session at all -- and the gap-stopped-primary rotation
- // can pick a peer that is simply down. Past the budget the session
- // is dropped and re-armed one step around the ring; an ordinary
lost
- // frame is re-requested long before that.
- if self.burn_metadata_repair_attempt() {
- let next_peer = next_transfer_peer(
- consensus.replica(),
- peer,
- consensus.replica_count(),
- consensus.primary_index(consensus.view()),
- );
- tracing::warn!(
+ if let Some((peer, nonce, session_from_op, to_op)) = stalled {
+ // What the walk has not reached, floored at the op this session
was
+ // armed for. Recomputing would ask for a different window than
the one
+ // reported missing: the merged-log scan opens above the snapshot
floor
+ // and its `committed_elsewhere` fallback reports ops below even
that,
+ // both already folded into `session.from_op`. It also carries the
+ // initial arm's snapshot clamp, so a retry cannot ask for
compacted ops.
+ let from_op = session_from_op.max(consensus.commit_min() + 1);
Review Comment:
Fixed. The retry no longer raises a merged-log session's floor to
`commit_min + 1`; it reopens at `session.from_op`, and rotation inherits it.
Extracted as `stalled_repair_from_op` with your case pinned:
`given_a_dropped_response_below_commit_min_when_retrying_should_still_ask_for_it`
(from_op 5, commit_min 6, retries from 5).
##########
core/shard/src/lib.rs:
##########
@@ -5225,6 +5261,85 @@ where
// `RangeEvicted` again if the primary checkpointed mid
// transfer -- that reraises through the same path, and
each
// round lifts the local floor, so it converges.
+ //
+ // Never as primary-elect. A transfer replaces
snapshot-shaped
+ // state wholesale, and this replica has a merged log
parked
+ // against that state naming ops it was just told it cannot
+ // serve; installing under it starts the view over a log
the new
+ // state no longer matches. Re-target instead, and let the
+ // view-change timeout escalate if nobody can serve it.
+ //
+ // DROP the session before returning. The serving peer
follows
+ // `RangeEvicted` with `RepairDone(from_op - 1)` on the
same
+ // nonce, and a `RepairDone` at or below `commit_min` walks
+ // nothing, so `repair_chunk_walked` is trivially true and
that
+ // arm re-requests at once: no tick gate, no debounce, no
attempt
+ // burned -- an unthrottled request/reply loop across two
pumps.
+ // Dropping first lands the trailing frame on the `is_none`
+ // guard, as it did before this arm existed.
+ if consensus.view_log_is_pending()
+ && consensus.is_primary_for_view(consensus.view())
+ {
+ tracing::warn!(
+ shard = self.id,
+ peer = header.replica,
+ retained_from = header.op,
+ local_commit = consensus.commit_min(),
+ "merged-log repair peer evicted the requested
range; \
+ re-targeting rather than transferring state mid
view change"
+ );
+ // Definitive, not a stall: this sender has said it
cannot
+ // serve the window, so rotate now rather than spend a
retry
+ // interval on a stream that will not come. Still
charge a
+ // round, so a quorum that all answer this way stops
asking
+ // instead of cycling the ring until the timeout.
+ if self.burn_metadata_repair_attempt() {
+ *self.metadata_repair.borrow_mut() = None;
+ tracing::warn!(
+ shard = self.id,
+ from_op = session.from_op,
+ to_op = session.to_op,
+ "merged-log repair exhausted its senders;
leaving the view \
+ change to its timeout"
+ );
+ return;
+ }
+ self.rotate_stalled_metadata_repair(
+ consensus,
+ header.replica,
+ session.from_op,
+ session.to_op,
+ )
+ .await;
+ return;
+ }
+
+ // The floor must also be ABOVE the op this replica needs.
A
+ // peer behind the requested window walks its serve range
off
+ // the end and answers `RangeEvicted` at the requested
floor
+ // itself, having retained nothing and evicted nothing;
+ // converting on that arms a transfer against a replica
with
+ // less state than this one and fences repair for a full
+ // transfer backoff. Drop the session and let the level
trigger
+ // re-request from the primary instead.
+ if header.op <= consensus.commit_min() + 1 {
+ tracing::warn!(
+ shard = self.id,
+ peer = header.replica,
+ retained_from = header.op,
+ local_commit = consensus.commit_min(),
+ "metadata repair peer retained nothing in the
requested range; \
+ re-requesting rather than converting to state
transfer"
+ );
+ *self.metadata_repair.borrow_mut() = None;
+ // Charged, not cleared. `gap_repair_peer` re-picks the
+ // primary deterministically, so clearing makes this
+ // request / `RangeEvicted` / re-arm cycle unbounded
at the
+ // debounce interval and puts the state-transfer
escalation
+ // this guard replaced out of reach.
+ self.burn_metadata_repair_attempt();
Review Comment:
Fixed. The arm now acts on exhaustion: past the budget it calls
`rotate_stalled_metadata_repair` instead of dropping the session for the level
trigger to re-arm against the same peer.
##########
core/shard/src/lib.rs:
##########
@@ -5069,15 +5093,19 @@ where
let Some(journal) = planes.0.journal.as_ref() else {
return;
};
- // Above the two returns below, not after them: only SILENCE should
- // age the stream, and an in-scope frame proves the peer is serving
- // whether or not this replica still needs the op it carries. The
- // ops a re-request re-serves are exactly the ones already held, so
- // counting accepted frames alone rotates away from a live peer.
- if let Some(session) = self.metadata_repair.borrow_mut().as_mut() {
- session.idle_ticks = 0;
- }
- self.note_metadata_repair_progress();
+ // Below the divergence and integrity returns, above the two under
it.
+ //
+ // Only silence should age the stream, and only a frame this
replica
+ // would have accepted proves anything is being served. A forked or
+ // corrupted frame is neither: the peer re-serves the same stored
bytes
+ // every re-request, so crediting those holds off the retry
forever.
+ //
+ // Still above the dedup return: a re-request re-serves ops already
+ // held, and a stream re-covering ground is still a stream.
+ //
+ // Clock only -- the frame has no sender and no nonce, so it
cannot be
+ // attributed. The budget is cleared from the terminator and the
walk.
+ self.note_metadata_repair_clock();
Review Comment:
Not fixed. Tying it needs sender or nonce on the frame, and there is nowhere
to put one: `RepairPrepareHeader` is `repr(transparent)` over `PrepareHeader`
and `identity_checksum` covers every byte but `checksum` and `view`. The budget
no longer moves on an unattributable frame, so the residue is one retry
interval on the clock, bounded because nothing re-requests from the old peer.
Happy to do the wire change (stamp the nonce in `reserved_frame`, zero it
before verify, same trick as the command byte) if you want it.
##########
core/simulator/src/bin/workload-fuzz.rs:
##########
@@ -132,11 +132,20 @@ struct Args {
/// empty shadow against empty committed state and agrees. `0` opts out.
#[arg(long, default_value_t = 1)]
min_commits: u64,
- /// Committed metadata ops that must have been witnessed by more than one
live
- /// replica, i.e. that exercised cross-replica agreement. Ignored below
two live
- /// replicas, where the property is untestable rather than untested. `0`
opts out.
+ /// Committed ops, on EITHER plane, that must have been witnessed by more
than
+ /// one live replica, i.e. that exercised cross-replica agreement. Ignored
below
+ /// two live replicas, where the property is untestable rather than
untested.
+ /// `0` opts out.
#[arg(long, default_value_t = 1)]
min_ops_compared: usize,
+ /// As `--min-ops-compared`, but METADATA ops only.
+ ///
+ /// Separate because a partition-only run satisfies the combined floor
while the
+ /// metadata oracle compares an empty chain against an empty chain and
agrees.
+ /// Default `0`: a partition-focused run legitimately commits no metadata,
so a
+ /// campaign that wants the metadata property tested asks for it.
+ #[arg(long, default_value_t = 0)]
Review Comment:
Fixed. Default is 1, matching the floor `--min-ops-compared` carried before
it counted both planes; partition-only campaigns opt out with 0.
##########
core/consensus/src/impls.rs:
##########
@@ -5404,3 +5458,236 @@ mod quorum_tests {
}
};
}
+
+#[cfg(test)]
+mod view_source_tests {
+ //! Who a primary-elect may ask for an op its merged log names. A
`DoViewChange`
+ //! suffix spans `commit..=op`, so the two selectors cover disjoint halves
of
+ //! the merged log and the split is what keeps a coverage gap under the
merged
+ //! commit point askable at all.
+
+ use super::*;
+ use crate::LocalPipeline;
+ use crate::view_change_quorum::{DvcSuffix, StoredDvc, dvc_record};
+
+ use crate::test_bus::NoopBus;
+
+ fn consensus() -> VsrConsensus<NoopBus, LocalPipeline> {
+ VsrConsensus::new(1, 0, 3, METADATA_GROUP, NoopBus,
LocalPipeline::new())
+ }
+
+ /// A sender whose suffix runs `commit..=op` with every body offered,
which is
+ /// the widest window a real `DoViewChange` can carry.
+ fn sender(replica: u8, log_view: u32, op: u64, commit: u64) -> StoredDvc {
+ let headers: Vec<PrepareHeader> = (commit..=op)
+ .rev()
+ .map(|op| PrepareHeader {
+ command: Command::Prepare,
+ op,
+ view: log_view,
+ ..Default::default()
+ })
+ .collect();
+ let present = (1u128 << headers.len()) - 1;
+ StoredDvc {
+ replica,
+ log_view,
+ op,
+ commit,
+ suffix: DvcSuffix::new(headers, 0, present),
+ }
+ }
+
+ fn record(consensus: &VsrConsensus<NoopBus, LocalPipeline>, senders:
[StoredDvc; 2]) {
+ let mut quorum =
consensus.do_view_change_from_all_replicas.borrow_mut();
+ for dvc in senders {
+ assert!(dvc_record(&mut quorum, dvc));
+ }
+ }
+
+ #[test]
+ fn
given_an_op_inside_the_suffixes_when_selecting_should_return_the_body_offers() {
+ let consensus = consensus();
+ record(&consensus, [sender(1, 5, 12, 10), sender(2, 4, 12, 10)]);
+
+ assert_eq!(
+ consensus.pending_view_body_sources(11),
+ vec![1, 2],
+ "both senders offer op 11, freshest log_view first"
+ );
+ }
+
+ #[test]
+ fn
given_an_op_below_every_commit_point_when_selecting_should_need_the_committers()
{
+ let consensus = consensus();
+ record(&consensus, [sender(1, 5, 12, 10), sender(2, 4, 12, 10)]);
+
+ assert!(
+ consensus.pending_view_body_sources(7).is_empty(),
+ "a suffix spans commit..=op, so it says nothing about op 7"
+ );
+ assert_eq!(
+ consensus.pending_view_commit_sources(7),
+ vec![1, 2],
+ "a sender that committed op 7 holds it or compacted it, and either
\
+ answer moves the requester forward"
+ );
+ }
+
+ #[test]
+ fn given_a_sender_behind_the_op_when_selecting_committers_should_skip_it()
{
+ let consensus = consensus();
+ record(&consensus, [sender(1, 5, 12, 10), sender(2, 6, 6, 5)]);
+
+ assert_eq!(
+ consensus.pending_view_commit_sources(7),
+ vec![1],
+ "replica 2 never committed op 7, so asking it wastes a retry
interval \
+ on a RangeEvicted it has no standing to send"
+ );
+ }
+
+ #[test]
+ fn
given_this_replica_in_the_quorum_when_selecting_should_never_return_self() {
+ let consensus = consensus();
+ record(&consensus, [sender(0, 5, 12, 10), sender(2, 4, 12, 10)]);
+
+ assert_eq!(
+ consensus.pending_view_commit_sources(7),
+ vec![2],
+ "a replica cannot repair from itself"
+ );
+ }
+}
+
+#[cfg(test)]
+mod recovery_barrier_tests {
+ //! The gate holding a restarted replica's reads and writes until the
recovered
+ //! WAL suffix re-commits. Boot arms it at the recovered head; only a view
that
+ //! settles that suffix's fate may move it, and only downward.
+
+ use super::*;
+ use crate::LocalPipeline;
+ use crate::test_bus::NoopBus;
+
+ /// A `StartView` at `view` announcing head `op` with commit point
`commit`.
+ fn start_view(view: u32, op: u64, commit: u64) -> Message<StartViewHeader>
{
Review Comment:
Fixed. One `make_start_view` in `test_bus` alongside `NoopBus`; both modules
use it and the local copies are gone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
