github-actions[bot] commented on code in PR #4429:
URL: https://github.com/apache/iggy/pull/4429#discussion_r4272746070
##########
core/shard/src/lib.rs:
##########
@@ -1489,10 +1489,10 @@ where
/// metadata group per node (precedent:
[`Self::metadata_transfer_attempts`]).
metadata_gap_ticks: Cell<u32>,
- /// Op the tick's commit walk last stopped on without moving, or `0`. The
- /// journal names it but cannot produce its body, so the gap probe counts
it
- /// as absent and lets repair fetch it. Cleared implicitly: any advance of
- /// `commit_min` makes it stop matching `commit_min + 1`.
+ /// Op last truncated after a `BodyMissing` walk stop, or `0`. The gap
probe
Review Comment:
nit: The field doc says this op was truncated, but the code records it
before the `truncate_from` attempt and keeps it after a failure. Say instead
that the record names the op the drop targeted.
##########
core/shard/src/lib.rs:
##########
@@ -10571,13 +10572,20 @@ where
commit_max = probe.commit_max,
"metadata commit walk parked over resident committed ops;
resuming"
);
- metadata.commit_journal().await;
- // Promotion above can give the pipeline ownership after the
- // stranded-commit driver ran. Verify an absent body before
- // removing any header from a walk that made no progress.
- let walked = consensus.commit_min();
- if walked == probe.commit_min {
- let stuck_op = walked.saturating_add(1);
+ // Truncate on `BodyMissing` alone. A walk that moved nothing
is
+ // not proof: `PipelineHead` moves nothing either, and dropping
Review Comment:
nit: `drop_unwalkable_metadata_entry` already returns early when any
pipeline head is resident, so a `PipelineHead` stop never reaches
`truncate_from`. State what this change adds instead, for example the explicit
stop reason or the same-tick truncation after a partial walk. Also at
`core/simulator/src/lib.rs:11161`.
##########
core/simulator/src/lib.rs:
##########
@@ -11150,3 +11150,117 @@ mod probe_answer_divergence_tests {
);
}
}
+
+#[cfg(test)]
+mod metadata_walk_stuck_detector_tests {
+ //! The metadata walk-stall detector in `tick_metadata` must not mistake
the
+ //! pipeline head for a header without a body.
+ //!
+ //! `commit_journal` stops, without moving `commit_min`, at the op the
+ //! pipeline holds, because committing it is `on_ack`'s job. A detector
that
+ //! read "the walk moved nothing" as "the body is unreadable" would
truncate
+ //! the WAL from that op, leaving the primary's pipeline with a committed
+ //! header whose prepare is gone, and the next commit driver would panic
+ //! with `committed prepare ... must be in journal`.
+
+ use consensus::Consensus;
+
+ use super::*;
+
+ /// Swarm seed whose network elects replica 1 primary of view 1 with a
+ /// committed pipeline head it has not yet walked.
+ const SEED: u64 = 1716;
+ const REPLICAS: u8 = 3;
+ const CLIENTS: [u128; 3] = [1, 2, 3];
+
+ const VIEW_0_PRIMARY: u8 = 0;
+ /// The replica the faults leave to win view 1.
+ const PRIMARY_ELECT: usize = 1;
+
+ /// Replica faults as `(step, replica, crash)`. The view-0 primary goes
+ /// down, which puts the view-1 change on the primary-elect, and comes back
+ /// before the primary-elect holds the inherited head (step ~722).
+ const FAULTS: [(usize, u8, bool); 2] =
+ [(91, VIEW_0_PRIMARY, true), (631, VIEW_0_PRIMARY, false)];
+
+ /// Past the inherited-head window, then the drain heals the cluster.
+ const FAULT_STEPS: usize = 800;
+ const DRAIN_STEPS: usize = 4_000;
+
+ /// Only the three `Register`s go through the metadata plane: no client
+ /// traffic, no metadata workload. Op 3 is the last of them.
+ const REGISTERED_OPS: u64 = 3;
+
+ #[test]
+ fn
given_primary_elect_inheriting_a_committed_head_when_the_tick_probes_the_walk_should_keep_its_prepare()
+ {
+
server_common::MemoryPool::init_pool(&server_common::MemoryPoolSettings {
+ enabled: false,
+ size: iggy_common::IggyByteSize::from(0u64),
+ bucket_capacity: 1,
+ });
+ let mut network = packet::PacketSimulatorOptions::swarm(SEED);
+ network.node_count = REPLICAS;
+ network.client_count = u8::try_from(CLIENTS.len()).expect("few
clients");
+ let mut sim = Simulator::new(usize::from(REPLICAS),
CLIENTS.iter().copied(), network);
+ sim.init_partition(IggyNamespace::new(1, 1, 0));
+ for id in CLIENTS {
+ sim.register_client_with_primary(&SimClient::new(id));
+ }
+
+ let mut inherited_head_seen = false;
+ for step in 0..FAULT_STEPS {
+ for &(at, replica, crash) in &FAULTS {
+ if at == step {
+ if crash {
+ sim.replica_crash(replica);
+ } else {
+ sim.replica_restart(replica);
+ }
+ }
+ }
+ sim.step();
+ inherited_head_seen |= primary_elect_holds_inherited_head(&sim);
+ }
+ // Without this a drifted seed that never builds the shape passes the
+ // drain assertion below without testing anything.
+ assert!(
+ inherited_head_seen,
+ "replica {PRIMARY_ELECT} never led view 1 with a committed
pipeline head at \
+ commit_min + 1; the seed no longer reaches the shape under test"
+ );
+ for _ in 0..DRAIN_STEPS {
+ sim.step();
+ }
+
+ // Every replica applies every registration: a dropped pipeline prepare
+ // must neither panic the primary nor leave any replica short of it.
+ for replica in 0..usize::from(REPLICAS) {
+ let consensus = sim
+ .metadata_consensus(replica)
+ .expect("shard 0 hosts the metadata plane");
+ assert!(
+ consensus.commit_min() >= REGISTERED_OPS,
+ "replica {replica} applied metadata only through op {}, short
of the \
+ {REGISTERED_OPS} registrations",
+ consensus.commit_min()
+ );
+ }
+ }
+
+ /// Whether the primary-elect is a normal view-2 primary whose pipeline
head
Review Comment:
nit: The helper doc says the check looks for a view-2 primary, but the code
accepts any `view() >= 1`. Change the doc to match the code, `view() >= 1`.
--
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]
