krishvishal commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3960243602
##########
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:
Fixed. Both ingest sites now use the same predicate as the sweep, extracted
as `consensus::repair_session_live` (Normal, or the primary-elect that parked
the merged log) so arming and ingest cannot drift again. Confirmed exactly as
described: `apply_repaired_prepare` and the partition arm of
`on_repair_range_reply` each bailed on bare `!is_normal()` and cleared
`partition.repair`, so the session died on its first inbound frame and the
level-triggered scan re-sent the window every tick.
##########
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:
Fixed. The budget is now charged per view: `metadata_repair_attempts`
carries the view its rounds belong to, and `metadata_repair_exhausted` only
refuses within that view. A later view is a new merged log from a new quorum,
so the senders that refused this one say nothing about it.
##########
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:
Fixed. Confirmed reachable, so it is no longer called a defect: downgraded
to `warn!`, and the doc now says a `set_commit_floor` jump past a live pipeline
reaches it and only a view change clears it. Also raised the simulator's
applied-head threshold from 5 to 2000 ticks, which would have failed runs on
that window. Separately: the missing `clear_pipeline` on `metadata.rs:1937` and
`complete_repair` looks like a real gap against the partition transfer path,
but I did not want to change state transfer blind. Happy to take it if you want.
--
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]
