krishvishal commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3957094766
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -6995,6 +6993,65 @@ mod tests {
);
}
+ /// `AckLevel::NoAck` stores apply on the primary only and never
replicate, so
+ /// which replicas hold an offset is not agreed and a committed delete can
+ /// legitimately find nothing. Erroring here fails the committed apply,
fences
+ /// the partition, and then crash-loops on every replay of the same op.
+ #[compio::test]
+ async fn
given_an_absent_offset_when_committing_a_delete_should_apply_without_fencing() {
+ let mut partition = test_partition();
+ assert!(
+ !partition.consensus().is_follower(),
+ "the role the old existence check errored on is the primary"
+ );
+
+ partition.stage_consumer_offset_delete(1, ConsumerKind::Consumer, 7);
+ partition
+ .apply_staged_consumer_offset_commit(1)
+ .await
+ .expect("a committed delete of an absent consumer offset must
apply");
+
+ partition.stage_consumer_offset_delete(2, ConsumerKind::ConsumerGroup,
9);
+ partition
+ .apply_staged_consumer_offset_commit(2)
+ .await
+ .expect("a committed delete of an absent group offset must apply");
+
+ assert!(
+ partition.fatal().is_none(),
Review Comment:
Fixed. Both new tests dropped; the `ConsumerGroup` arm moved into master's
`given_absent_offset_file_when_delete_commits_should_skip_directory_sync`,
which does drive `handle_committed_entries`.
##########
core/simulator/src/lib.rs:
##########
@@ -135,6 +135,31 @@ pub(crate) struct PartitionConsensusState {
pub commit_min: u64,
}
+/// A pipeline head the commit walk is holding on: covered by the commit
frontier,
+/// but not the op the state machine is next owed.
+///
+/// What `drain_committable_prefix` / `peek_committable_head` refuse to drain.
Legit
+/// and transient right after a partition promotion, whose `RebuildPipeline`
seeds
+/// above an apply backlog the bounded journal walk clears over the following
+/// sweeps; permanent means the ops between are gone and nothing is repairing
them.
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct CommitPrefixHole {
+ pub head_op: u64,
+ pub commit_min: u64,
+ pub commit_max: u64,
+}
+
+impl CommitPrefixHole {
+ fn read(head: Option<PrepareHeader>, commit_min: u64, commit_max: u64) ->
Option<Self> {
+ let head = head?;
+ (head.op <= commit_max && head.op != commit_min + 1).then_some(Self {
Review Comment:
Fixed. `CommitHoldKind::{MissingOps, AppliedHead}` splits them: separate
thresholds (2000 vs 5 ticks), separate messages, and only the missing-op count
resets on `commit_min` advancing.
##########
core/simulator/src/workload/invariants.rs:
##########
@@ -102,6 +141,91 @@ impl Invariants {
self.state_checker.check(sim, seed);
}
+ /// Catch a metadata primary permanently shut behind its own recovery
barrier.
+ ///
+ /// The shape `VsrConsensus::redecide_recovery_barrier` fixes, caught from
the
+ /// outside: a primary that can never clear its barrier drops every
request as
+ /// `NotReady`, which otherwise surfaces only as an unexplained stall.
+ ///
+ /// # Panics
+ /// When a `Normal` metadata primary sits below its barrier for
+ /// [`RECOVERY_BARRIER_WEDGE_TICKS`] consecutive ticks.
+ fn check_recovery_barrier(&mut self, sim: &Simulator, seed: u64,
replica_idx: u8) {
+ let Some(consensus) = sim.replicas[usize::from(replica_idx)].shards[0]
+ .plane
+ .metadata()
+ .consensus
+ .as_ref()
+ else {
+ return;
+ };
+ let gated = consensus.is_primary()
+ && !consensus.has_ceded_primaryship()
+ && consensus.is_normal()
+ && consensus.commit_max() < consensus.recovery_barrier();
Review Comment:
Fixed. The predicate now trips on either gate: `commit_max < barrier`
(admission) or `commit_min < barrier` (the HTTP read gate).
##########
core/simulator/src/bin/workload-fuzz.rs:
##########
@@ -574,13 +576,17 @@ fn run_quiesce_phase(
proved nothing about entity state (seed={seed:#x})"
);
let live = usize::from(replicas) - sim.crashed.len();
+ // Either plane satisfies it: a partition-plane run commits almost no
metadata,
+ // so the metadata count alone called every such run vacuous.
+ let compared = convergence.ops_compared +
convergence.partition_ops_compared;
assert!(
- args.min_ops_compared == 0 || live < 2 || convergence.ops_compared >=
args.min_ops_compared,
- "--min-ops-compared {}: {live} replicas live but only {} op(s)
witnessed \
- by more than one, so cross-replica agreement went untested \
- (seed={seed:#x})",
+ args.min_ops_compared == 0 || live < 2 || compared >=
args.min_ops_compared,
Review Comment:
Fixed. Added `--min-metadata-ops-compared` as a separate per-plane floor and
corrected the `--min-ops-compared` help to say "either plane".
##########
core/shard/src/lib.rs:
##########
@@ -4972,6 +4975,9 @@ where
if !in_scope {
return;
}
+ // The serving peer is answering. Clears the stall clock and the
budget
+ // so a window served in chunks cannot rotate off a healthy stream.
+ self.note_metadata_repair_progress();
Review Comment:
Not fixed as proposed. There is nowhere to put a nonce:
`RepairPrepareHeader` is `repr(transparent)` over `PrepareHeader` and
`identity_checksum` covers every byte but `checksum` and `view`. Closed the
consequence instead: only attributable evidence clears the budget, so a stale
frame costs one retry interval and cannot make rotation unreachable.
--
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]
