numinnex commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3949102908
##########
core/consensus/src/plane_helpers.rs:
##########
@@ -446,18 +456,35 @@ where
P: Pipeline<Entry = PipelineEntry>,
{
let commit = consensus.commit_max();
+ let commit_min = consensus.commit_min();
+ let replica = consensus.replica();
let mut drained = Vec::new();
consensus.with_pipeline_mut(|pipeline| {
+ let mut next = commit_min + 1;
while let Some(head_op) = pipeline.head().map(|entry| entry.header.op)
{
if head_op > commit {
break;
}
+ if head_op != next {
Review Comment:
`head_op != next` also matches `head_op <= commit_min`, and that arm is
terminal. `commit_journal` walks to `commit_max` (`metadata.rs:3645`,
`iggy_partition.rs:3620`), applies this op from the journal, and
`advance_commit_min` steps past it. The only popper is `pop_committed_prepare`,
whose sole caller (`metadata.rs:2843`) sits inside the loop this hold gates, so
nothing ever pops it: prepare slots leak while `is_caught_up_primary` still
reports caught up. Pre-PR this interleaving hit the assert at `impls.rs:1608`.
Suggest pop-and-discard when `head_op <= commit_min`, holding only when
`head_op > commit_min + 1`.
##########
core/consensus/src/plane_helpers.rs:
##########
@@ -482,15 +510,33 @@ 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 next = consensus.commit_min() + 1;
+ let head = consensus
.pipeline_head_header()
- .filter(|header| header.op <= commit)
+ .filter(|header| header.op <= commit)?;
+ if head.op != next {
Review Comment:
Same conflation as the drain site, and harder here: `peek_committable_head`
cannot pop, so once `head.op <= commit_min` this returns `None` permanently and
`commit_committable_prefix` never runs again. Wire replies exist only on that
path (`metadata.rs:2815-2967`), so committed metadata ops are applied by the
journal walk and never answered, and each entry's `reply_sender`
(`impls.rs:198`) is neither taken nor dropped, so in-process awaiters get no
`Canceled` either. The pop needs to be a separate call the caller makes before
peeking.
##########
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:
This sits above the divergence return (`:5000`) and
`verify_prepare_integrity` (`:5011`); master had it below both so that only
silence aged the stream. A peer serving forked or corrupt in-window frames now
zeroes `idle_ticks`, so `retry_stalled_metadata_repair` never reaches its stall
path, `burn_metadata_repair_attempt` is never called at all, and
`REPAIR_MAX_STALL_RETRIES` is unreachable. Rotation is not merely reset here,
it cannot run. On the `disagrees` path this is deterministic rather than
transient. Suggest restoring master's position below both returns.
##########
core/shard/src/lib.rs:
##########
@@ -5499,9 +5541,12 @@ where
// from the evicted ring or the flushed segments.
let missing = {
let journal = partition.log.journal();
- first_op_not_covered(&pending, consensus.commit_min(), |op| {
- journal.inner.header_by_op(op)
- })
+ first_op_not_covered(
+ &pending,
+ consensus.commit_min(),
+ consensus.commit_min(),
Review Comment:
This lowers the partition floor from `max(pending.commit_max, commit_min+1)`
to `commit_min+1` unconditionally, and that range is one eviction has already
cleared: `flush_committed_messages` evicts `committed_prefix(commit_max)`
(`iggy_partition.rs:4029`, `:4243`) while `header_by_op` reads the resident vec
only. A primary-elect then parks on a committed, durable op with no fill path,
since this site arms no repair and `group_is_gap_stopped` requires
`probe.normal`. The comment at `:5531-5541` names this exact hazard. Fix needs
both halves: probe `repair_headers_in(floor..=pending.op_head)`, and pass
`repair_retained_from()` minus one as `repair_floor` (the evicted ring is
capped at 4096, so a ring-aware probe alone still parks).
##########
core/consensus/src/plane_helpers.rs:
##########
@@ -435,8 +435,18 @@ where
/// Drain and return committable prepares from the pipeline head.
///
-/// Entries are drained only from the head and only while their op is covered
-/// by the current commit frontier.
+/// Entries are drained from the head, while covered by the commit frontier,
and
+/// only as a contiguous run starting at the next op owed to the state machine.
+///
+/// Callers `advance_commit_min` per entry, so a run starting above
+/// `commit_min + 1` or breaking partway hits that counter's sequential-advance
+/// assert. Pipeline-side twin of `commit_journal`'s gap-stop.
+///
+/// Reported, not asserted: a promoted partition primary reaches it
legitimately,
+/// `RebuildPipeline` seeding above `commit_min` while the walk that catches
it up
+/// runs `COMMIT_WALK_OPS_MAX` ops per call. `commit_journal`'s journal
fallback
Review Comment:
This premise can be false, which matters because the assert downgrade rests
on it. `commit_messages` runs from `commit_partition_entry` before
`advance_commit_min`, and `committed_prefix(commit_max)` (`journal.rs:526`)
returns the contiguous resident prefix up to the cluster frontier, so eviction
(`iggy_partition.rs:4243`) also clears the un-drained remainder past the 64-op
cap. `committed_headers_from` (`journal.rs:892`) has no ring read, so the
fallback can return empty and the walk wedges at `commit_min`. Worth confirming
before treating it as the backstop.
##########
core/consensus/src/plane_helpers.rs:
##########
@@ -446,18 +456,35 @@ where
P: Pipeline<Entry = PipelineEntry>,
{
let commit = consensus.commit_max();
+ let commit_min = consensus.commit_min();
+ let replica = consensus.replica();
let mut drained = Vec::new();
consensus.with_pipeline_mut(|pipeline| {
+ let mut next = commit_min + 1;
while let Some(head_op) = pipeline.head().map(|entry| entry.header.op)
{
if head_op > commit {
break;
}
+ if head_op != next {
+ tracing::warn!(
Review Comment:
`warn!` per ack on a state this same doc block calls legitimate and
transient. The two enclosing call sites already settled this level:
`shard/lib.rs:7565` and `:9734` both chose `debug!` with the comment that it
would otherwise be one line per group per tick. The message is also wrong in
both arms: `commit_journal` closes the backlog case and only a pop closes the
applied-head case, so "until repair refills it" names a remedy that applies to
neither.
##########
core/shard/src/lib.rs:
##########
@@ -5658,22 +5701,108 @@ where
consensus.group(),
)
.await;
- } else {
- // `from_op` past `to_op` without `commit_min` reaching it: the
- // primary-elect window above starts at the merged log's commit
- // point, which can sit above what this replica has walked. The
- // top-of-tick check closes the ordinary case; this closes the
- // one it cannot see.
- tracing::info!(
+ }
+ }
+ }
+
+ /// Re-arm a repair session that spent its stall budget against another
replica.
+ ///
+ /// A session pins its peer and fences every arming site while it stands,
so a
+ /// peer that cannot answer wedges the walk harder than having no session
at
+ /// all. Past the budget the session is dropped and re-armed one step on;
an
+ /// ordinary lost frame is re-requested long before that. Mirrors the
partition
+ /// rotation in [`Self::tick_partitions`].
+ ///
+ /// Two rings, because two things decide who can serve. A `Normal` backup
is
+ /// repairing its committed tail and any replica ahead of it will do, so it
+ /// walks the cluster preferring the primary. A primary-elect is repairing
+ /// toward a merged log, and only the `DoViewChange` senders that named
the op
+ /// can serve it: walking the whole ring lands on a replica that answers
+ /// `RangeEvicted` for a range it never held.
+ #[allow(clippy::future_not_send)]
+ async fn rotate_stalled_metadata_repair<P>(
+ &self,
+ consensus: &VsrConsensus<B, P>,
+ peer: u8,
+ from_op: u64,
+ to_op: u64,
+ ) where
+ B: MessageBus,
+ P: Pipeline<Entry = consensus::PipelineEntry>,
+ {
+ *self.metadata_repair.borrow_mut() = None;
+ self.metadata_repair_attempts.set(0);
Review Comment:
This contradicts both docs that justify the counter living on the shard
rather than the session: `:1565-1571` and `:6665-6670` state it has to outlive
the session, or the rotation that mints a new one would reset the count and
re-target forever without giving up on a peer. Rotation is that reset, and
every other session-drop path zeroes it too (`:4980`, `:5205`, `:5631`,
`:5682`), so it now bounds one round. Either drop this line, or move the field
onto `MetadataRepairSession` and rewrite both docs.
##########
core/shard/src/lib.rs:
##########
@@ -5163,6 +5160,51 @@ 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 has just been told it
+ // cannot serve; installing under it would start the view
over
+ // a log the new state no longer matches. The honest
answer is
+ // that another replica holds the committed prefix, so
leave
+ // the session for the stall rotation to re-target and let
the
+ // view-change timeout escalate if nobody can serve it.
+ 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; \
+ waiting for another sender rather than
transferring \
+ state mid view change"
+ );
+ 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 retains the requested floor
but served \
+ nothing; re-requesting rather than converting to
state transfer"
+ );
+ *self.metadata_repair.borrow_mut() = None;
+ self.metadata_repair_attempts.set(0);
Review Comment:
Zeroing the budget here means no round is ever charged, and
`gap_repair_peer` (`:10363`) deterministically re-picks the primary. So the
sequence is request, `RangeEvicted`, drop, re-arm the same peer, repeating at
the debounce interval with no bound, and the state-transfer escalation this
guard replaced is now unreachable. Charging the round instead of resetting it
keeps the escalation path alive. The log text also disagrees with the comment
above it, which says the peer retained nothing.
##########
core/shard/src/lib.rs:
##########
@@ -9926,6 +10059,37 @@ where
}
}
+/// Lowest op of a primary-elect's merged log this replica can be held to.
+///
+/// The merged commit point is what the cluster committed, `commit_min` what
this
+/// replica applied; they diverge exactly when the local prefix has a hole. The
Review Comment:
They also diverge on plain apply lag with no hole present, and that is the
common case this `min` actually fires on: a contiguously lagging primary-elect
has `commit_min < pending.commit_max`. So the widened floor buys nothing there
while still taking the eviction exposure at the call site. Suggest wording it
as diverging whenever this replica has not applied the merged commit point,
hole or plain lag.
##########
core/shard/src/lib.rs:
##########
@@ -5658,22 +5701,108 @@ where
consensus.group(),
)
.await;
- } else {
- // `from_op` past `to_op` without `commit_min` reaching it: the
- // primary-elect window above starts at the merged log's commit
- // point, which can sit above what this replica has walked. The
- // top-of-tick check closes the ordinary case; this closes the
- // one it cannot see.
- tracing::info!(
+ }
+ }
+ }
+
+ /// Re-arm a repair session that spent its stall budget against another
replica.
+ ///
+ /// A session pins its peer and fences every arming site while it stands,
so a
+ /// peer that cannot answer wedges the walk harder than having no session
at
+ /// all. Past the budget the session is dropped and re-armed one step on;
an
+ /// ordinary lost frame is re-requested long before that. Mirrors the
partition
+ /// rotation in [`Self::tick_partitions`].
+ ///
+ /// Two rings, because two things decide who can serve. A `Normal` backup
is
+ /// repairing its committed tail and any replica ahead of it will do, so it
+ /// walks the cluster preferring the primary. A primary-elect is repairing
+ /// toward a merged log, and only the `DoViewChange` senders that named
the op
+ /// can serve it: walking the whole ring lands on a replica that answers
+ /// `RangeEvicted` for a range it never held.
+ #[allow(clippy::future_not_send)]
+ async fn rotate_stalled_metadata_repair<P>(
+ &self,
+ consensus: &VsrConsensus<B, P>,
+ peer: u8,
+ from_op: u64,
+ to_op: u64,
+ ) where
+ B: MessageBus,
+ P: Pipeline<Entry = consensus::PipelineEntry>,
+ {
+ *self.metadata_repair.borrow_mut() = None;
+ self.metadata_repair_attempts.set(0);
+
+ if consensus.view_log_is_pending() &&
consensus.is_primary_for_view(consensus.view()) {
+ let sources = metadata_view_repair_sources(consensus, from_op);
+ let Some(next_peer) = sources.into_iter().find(|candidate|
*candidate != peer) else {
Review Comment:
`find` over a peer-independent, stably sorted list is not a ring walk:
`[A,B,C]` yields B when peer is A and A when peer is B, so C is never tried and
the ring property `next_transfer_peer` had is lost. `:5737` also resolves
sources for `from_op`, the scan floor, while the initial arm resolves for
`missing_op` (`:6037`), so rotation can target replicas that never named the
hole. Suggest locating `peer`'s index in `sources` and taking the next
wrapping, and passing `missing_op`.
##########
core/consensus/src/impls.rs:
##########
@@ -1729,6 +1729,33 @@ impl<B: MessageBus, P: Pipeline<Entry = PipelineEntry>>
VsrConsensus<B, P> {
self.recovery_barrier.set(required_commit);
}
+ /// Re-decide the barrier against a log head the cluster just settled.
+ ///
+ /// Boot arms it at the recovered journal head: those ops were acked
before the
+ /// restart, so admitting writes before they re-commit rolls back committed
+ /// history. It otherwise clears only by `commit_max` passing it, which
never
+ /// happens when a view change discards the suffix instead of
re-committing it.
+ /// The boot re-pipeline already ran, so nothing re-prepares those ops,
+ /// `is_caught_up_primary` stays shut, and the primary drops the very
requests
+ /// that would raise `commit_max`.
+ ///
+ /// Call this wherever the head is authoritatively re-decided: a merged
log at
+ /// view start, an adopted `StartView`. `head` lowers the barrier when the
view
+ /// truncated the suffix, keeps it when the suffix survived.
+ ///
+ /// Lowered, never cleared. `is_caught_up_primary` reads it against
`commit_max`
+ /// and a met barrier costs it nothing, but `await_recovery_barrier` reads
it
+ /// against `commit_min`, and adoption raises `commit_max` before walking
the
+ /// suffix into the state machine. Zeroing a met barrier would open that
read
+ /// gate over the unapplied window it exists to hold.
+ pub fn redecide_recovery_barrier(&self, head: u64) {
+ let barrier = self.recovery_barrier.get();
+ if barrier == 0 {
+ return;
+ }
+ self.recovery_barrier.set(barrier.min(head));
Review Comment:
"Lowered, never cleared" does not hold at `head == 0`, where
`barrier.min(0)` is 0. That matters because `barrier_state`
(`core/server/src/http/reads.rs:243`) is `barrier == 0 || commit_min >=
barrier`, so a zeroed barrier bypasses the `commit_min` gate entirely rather
than being merely met. `announced` is the wire `StartViewHeader.op`, returned
verbatim on suffix-decode failure (`:3243`), `validate` has no `op >= 1` check,
and the low-op guard at `:3356` binds only when `msg_view == log_view`. Suggest
`if head == 0 { return; }`. Separately, making the barrier mutable post-boot
breaks `reads.rs:279`, which captures it once outside a loop that then polls
only `commit_min`, so an in-flight reader waits on a value that no longer
exists.
##########
core/shard/src/lib.rs:
##########
@@ -11247,7 +11416,7 @@ mod view_coverage_tests {
committed_elsewhere: Vec::new(),
};
let held = [sealed(100, 1), sealed(99, 7), sealed(98, 1)];
- let missing = first_op_not_covered(&pending, 0, |op| {
+ let missing = first_op_not_covered(&pending, 0, pending.commit_max,
|op| {
Review Comment:
This case and `:11443`, `:11448`, `:11490` pass `pending.commit_max` as
`commit_min`, which collapses `merged_log_scan_floor` back to master's floor,
so four of the five cases assert the old behaviour. The one case that exercises
the widened range stops on its first probe. Nothing covers a held run between
`commit_min + 1` and the hole, which is exactly the span that carries the scan
cost and the eviction exposure.
##########
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:
This assert cannot fail, so the test's "without_fencing" half is not
exercised. `fatal` is written only at `:2682`, `:3464` and `:4361`, and `:4361`
is reachable only through `handle_committed_entries` /
`commit_partition_entry`, which neither new test drives; both enter at
`apply_staged_consumer_offset_commit`. Routing the committed delete through
`handle_committed_entries` would make it real. Worth adding the
`apply_consumer_offset_no_ack` absent-offset arm at `:2278` too, since that is
the client-visible half of the change (silent no-reply becomes a committed
reply).
##########
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:
This gates on `commit_max`, the write gate, but the reason
`redecide_recovery_barrier` refuses to clear a met barrier is the
`commit_min`-based read gate at `core/server/src/http/reads.rs:243`. A barrier
met by `commit_max` and never by `commit_min` therefore 503s every HTTP read
indefinitely, and this invariant cannot observe it. Adding a `commit_min <
recovery_barrier()` arm would cover the half the fix exists for.
##########
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:
`head.op != commit_min + 1` also matches `head.op <= commit_min`, a head the
walk has already passed, and the panic text at `invariants.rs:217` then asserts
that the ops between are missing, which is the wrong diagnosis for that arm.
Two distinct wedge shapes share one type, one counter and one message.
Splitting them is a prerequisite for a correct threshold in either: the
applied-head arm never self-clears and wants a near-zero threshold, while the
real-hole arm wants a reset on `commit_min` progress. Note a blanket reset on
`commit_min` advance would be wrong on the conflated counter, since in the
applied-head arm the head is frozen while `commit_min` keeps climbing.
##########
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:
Summing both planes drops the metadata-plane guarantee: a run that compared
zero metadata ops now satisfies `--min-ops-compared 1`, which is the property
that kept the metadata oracle non-vacuous. The clap help at `:135-138` also
still reads "Committed metadata ops", so the operator-facing flag describes
behaviour it no longer has. Suggest keeping a per-plane floor, for example a
second `--min-metadata-ops-compared`, and correcting the help text.
##########
core/shard/src/lib.rs:
##########
@@ -10630,9 +10794,14 @@ const fn header_is_view_entry(local: &PrepareHeader,
canonical: &PrepareHeader)
/// Neither can diverge from the merged log (a committed or compacted op is the
/// quorum's op), and no repair puts the journal entry back, so demanding one
parks
/// the view change forever.
+///
+/// Opens at [`merged_log_scan_floor`]: the merged commit point alone would
declare
+/// the log serveable over a local gap, promoting a replica whose
`CommitJournal`
+/// gap-stops below where `RebuildPipeline` seeds.
fn first_op_not_covered(
pending: &MergedLog,
repair_floor: u64,
+ commit_min: u64,
Review Comment:
Two adjacent same-typed positional params with no param docs, where swapping
them compiles. The PR is its own proof: `:5547` passes `consensus.commit_min()`
twice, while `:5996` passes `snapshot_op` then `commit_min` and a swap there
would silently re-floor the metadata scan. A named `ScanFloor { repair_floor,
commit_min }` or newtypes would also make the duplication at `:5547` reviewable
rather than two identical arguments.
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -2081,10 +2081,27 @@ where
.is_some_and(|&high_water| offset <= high_water)
}
- fn apply_consumer_offset_commit(
- &self,
- pending: PendingConsumerOffsetCommit,
- ) -> Result<(), IggyError> {
+ /// Note a committed delete that found no offset to remove.
+ ///
+ /// Expected wherever the paired `AckLevel::NoAck` store never replicated,
so a
+ /// diagnostic and not a fault. Still logged: on a replica that did serve
the
+ /// store it is the first symptom of a lost apply.
+ fn log_absent_offset_delete(&self, kind: &str, id: u64) {
Review Comment:
"Still logged" does not hold in a default deployment: `debug!` against the
shipped `level = "info"` (`core/server/config.toml:384`, with the
`EnvFilter::new("INFO")` fallback at `server_common/src/log/logger.rs:186`)
means this signal is never emitted, so the rationale for dropping the error is
not backed by an observable replacement. `kind: &str` also hand-writes the
strings `impl Display for ConsumerKind` already produces with `pending.kind` in
scope, `u64::from(id)` widens two `u32` ids, and this bypasses the
`emit_partition_diag` schema the removed WARN used. Taking `ConsumerKind` and
`u32` through `emit_partition_diag`, at WARN when not a follower, closes all
four.
--
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]
