numinnex commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3960153584
##########
core/shard/src/lib.rs:
##########
@@ -5967,7 +6169,20 @@ where
// Stream already running; the stall retry covers it drying up.
return;
}
- let sources = consensus.pending_view_body_sources(missing_op);
+ // Level-triggered, so it would re-arm against the head of the same
source
+ // list next tick -- including the sender that just answered
`RangeEvicted`,
+ // which is how the budget got spent. Once every sender has been asked
and
+ // charged, asking again is not progress; the view-change timeout is.
+ if self.metadata_repair_attempts.get() >
partitions::REPAIR_MAX_STALL_RETRIES {
Review Comment:
`metadata_repair_attempts` has no view fence, and after this change nothing
routine resets it: the only clear left is `note_metadata_repair_walked`
(`:6873`), which needs a live session that walked or was superseded. The
primary-elect `RangeEvicted` arm at `:5296` burns a round per refusing sender
and drops the session once the budget is exhausted, so no session stands to be
superseded on the next view change.
So once the budget is spent, this replica refuses to arm merged-log repair
for every subsequent view for the life of the process — and a primary-elect
cannot clear it another way, since `group_is_gap_stopped` requires
`probe.normal`. A node that keeps winning elections never repairs. Suggest
resetting the budget when a new merged log is parked (or on view advance), not
only on walk evidence.
##########
core/consensus/src/plane_helpers.rs:
##########
@@ -482,15 +514,64 @@ where
/// revalidates that the head is still this exact entry before popping and
/// applying it. A driver dropped at an await strands nothing; a sibling driver
/// that committed the op first fails the caller's revalidation and re-peeks.
+///
+/// Bounded below for the reason [`drain_committable_prefix`] is, and reported
+/// rather than asserted for the same one. Holding is safe: a shard pump's
panic is
+/// swallowed by `compio::runtime::spawn`, while `tick_metadata` re-arms
repair on
+/// the level.
pub fn peek_committable_head<B, P>(consensus: &VsrConsensus<B, P>) ->
Option<PrepareHeader>
where
B: MessageBus,
P: Pipeline<Entry = PipelineEntry>,
{
let commit = consensus.commit_max();
- consensus
+ let commit_min = consensus.commit_min();
+ let head = consensus
.pipeline_head_header()
- .filter(|header| header.op <= commit)
+ .filter(|header| header.op <= commit)?;
+ if head.op != commit_min + 1 {
+ report_uncommittable_head(consensus.replica(), head.op, commit_min,
commit, 0);
+ return None;
+ }
+ Some(head)
+}
+
+/// Log a held pipeline head, with the remedy for the arm it is in.
+///
+/// `debug`, not `warn`, matching `tick_partitions` / `tick_metadata`: a hold
is the
+/// steady state for a whole rejoin, so `warn` is one line per group per tick.
A
+/// hold that never clears is caught by a simulator invariant, not by this
line.
+fn report_uncommittable_head(
+ replica: u8,
+ head_op: u64,
+ commit_min: u64,
+ commit_max: u64,
+ drained: usize,
+) {
+ if head_op > commit_min {
+ tracing::debug!(
+ replica,
+ head_op,
+ expected_op = commit_min + 1,
+ commit_min,
+ commit_max,
+ drained,
+ "committable head sits above a hole in the committed prefix;
holding the commit \
+ walk until the ops below it are journaled"
+ );
+ } else {
+ // Unreachable while both journal walks stop below the head, so this
is a
Review Comment:
This arm is reachable, so the `error!` below reads as a defect on a state
that can legitimately occur. `set_commit_floor` raises `commit_min` past a live
pipeline without clearing it: the metadata state-transfer install at
`core/metadata/src/impls/metadata.rs:1937` has no `clear_pipeline()`, and that
install's own comment at `:1804` says a prepare can be driven during the
superblock await, so "should not be committing" is not an invariant it rests
on. `IggyPartition::complete_repair`
(`core/partitions/src/iggy_partition.rs:6903`) has the same shape.
The partition state-transfer path does clear, and says why
(`core/partitions/src/state_transfer.rs:3148-3164`): "a bare rewind would turn
the silent desync into a shard panic". On metadata a `snapshot_seq` above the
pipeline tail also risks the release-active `assert_eq!` at
`core/consensus/src/impls.rs:534`.
##########
core/shard/src/lib.rs:
##########
@@ -8582,6 +8857,82 @@ where
true
}
+ /// Repair a primary-elect's merged log before it starts the view.
+ ///
+ /// Sibling of [`Self::maybe_request_partition_repair`], which refuses
outside
+ /// `Normal` because its window comes from the live commit frontier. This
window
+ /// comes from the parked merged log, so it runs in `ViewChange` for the
replica
+ /// that parked it. Without it the coverage scan in
+ /// [`Self::start_pending_partition_view`] reports an op nothing ever
fetches --
+ /// the sweep's gap detector needs `probe.normal` too -- and only the
+ /// view-change timeout moves the replica.
+ ///
+ /// `avoid` is the peer a stall just gave up on, so rotation lands on a
+ /// different sender instead of the head of the same list.
+ #[allow(clippy::future_not_send)]
+ async fn request_partition_view_repair(
Review Comment:
This session runs in `ViewChange` by design, but the two sites that ingest
its data were not taught that, so nothing it fetches can land.
`IggyPartition::apply_repaired_prepare`
(`core/partitions/src/iggy_partition.rs:6732`) and the partition arm of
`on_repair_range_reply` (`core/shard/src/lib.rs:5397`) both bail on a bare
`!is_normal()` *and* clear `partition.repair`, so the first inbound prepare or
terminator discards the frame and drops the session. The sweep already carries
the right predicate at `:7541` (`session_live = consensus_normal ||
repairing_view`); these two need the same one.
Net effect looks worse than the park it replaces: the coverage scan is
level-triggered per tick, so a parked primary-elect re-arms and re-sends the
full window at tick rate while the peer serves chunks that are all discarded,
and the `avoid` rotation at `:7636` is unreachable because the session never
survives to stall.
--
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]
