numinnex commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3955629873
##########
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:
Partition twin of the peek gate, and it defeats what
`iggy_partition.rs:4285` documents. Holding makes `drained.is_empty()` true, so
`commit_journal` falls through to `collect_committable_from_journal` ->
`committed_headers_from(commit_min+1, commit_max, 64)` (`journal.rs:876`),
which walks straight through the held head and advances past it with
`send_client_replies: false` (`iggy_partition.rs:4978`).
After that overshoot `on_ack` returns at `:4234` on every ack, so the
partition never ships a client reply again, and each stranded entry leaves a
`receiver.await` parked with no timeout (`shard/lib.rs:3853`). Same cap fix as
the peek site.
##########
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:
`head.op != next` also matches `head.op <= commit_min`, and that arm is
terminal. `commit_journal` (`metadata.rs:3644`) is bounded by `commit_max`
alone, never by the pipeline head, so after a backlog past
`COMMIT_WALK_OPS_MAX` it applies the seeded head out of the journal and
advances `commit_min` past it. `pop_committed_prepare`'s only caller
(`metadata.rs:2843`) sits inside this loop, so nothing ever pops: wire replies
are never built, `take_reply_sender()` neither fires nor drops, and
`is_caught_up_primary` keeps reporting healthy on `commit_min == commit_max`
until the pipeline fills and dispatch shuts.
Suggest capping both journal walks at `pipeline_head_op - 1`, with an absent
head meaning *no cap* — the pipeline is primary-only, so a naive
`saturating_sub(1)` on `head = 0` would freeze every backup's walk.
##########
core/shard/src/lib.rs:
##########
@@ -5550,9 +5592,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:
Both args are `consensus.commit_min()`, so `merged_log_scan_floor(p,
cm).max(repair_floor + 1)` is `cm + 1` for all inputs: `pending.commit_max`
never participates and master's `max(pending.commit_max, cm+1)` floor is gone.
That lands the scan inside the evicted window. `header_by_op`
(`journal.rs:760`) reads the resident vec only, while `commit_messages` evicts
`committed_prefix(commit_max)` — the cluster frontier, not `commit_min` — so a
primary-elect with an apply backlog reads `None` at `cm+1`, reports a false
`missing_op`, and parks in `ViewChange`. This site arms no repair and
`group_is_gap_stopped` needs `probe.normal`, so there is no fill path. It also
drops a structural bound: `op_head - commit_max <= 127` (`dvc_merge.rs:115`)
capped master's scan at 128 ops; this is 127 + an unbounded backlog of
whole-vec probes per tick, with `commit_min` frozen while parked.
Probing ring+resident and arming repair (or ceding primaryship) would fix
both halves; a `repair_retained_from()` floor alone would declare coverage for
ops the journal cannot serve.
##########
core/shard/src/lib.rs:
##########
@@ -5214,6 +5211,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()
Review Comment:
Returning with the session intact lets the trailing `RepairDone` run. The
nothing-retained serve arm sends `RangeEvicted` and then `RepairDone(from_op -
1)` on the same nonce (`:4780`, `:4793`); with a primary-elect `from_op <=
commit_min + 1`, `commit_journal` moves nothing and `repair_chunk_walked(C, C,
<=C)` (`:10465`) is true, so `:5180` re-requests immediately — no tick gate, no
debounce, no attempt burn.
That is an unthrottled request/reply loop across both shard pumps until the
view-change timeout escalates. Pre-PR the `RangeEvicted` arm dropped the
session first, so the trailing `RepairDone` hit `let Some(session) ... else
return`. Suggest fencing or dropping the session in this arm, or gating the
re-request on real chunk progress.
##########
core/shard/src/lib.rs:
##########
@@ -5023,6 +5026,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 now sits above the divergence return (`:5044`),
`verify_prepare_integrity` (`:5054`) and the dedup return (`:5064`); master had
it below all three (`be3663805:5060-5067`, "only SILENCE should age the
stream").
That makes rotation unreachable rather than merely deferred: a stall round
burns attempts 0->1, `1 > REPAIR_MAX_STALL_RETRIES` fails, the re-request draws
the same stored bytes back, and the first in-scope frame zeroes the budget
again — so the count oscillates 1->0 and never reaches 4. Deterministic on the
`disagrees` path, since the peer re-serves the same bytes. Suggest resetting
`idle_ticks` here but charging the budget only on an accepted frame.
--
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]
