krishvishal commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3957097053
##########
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
Review Comment:
Fixed. Same arm as the trailing-`RepairDone` fix: it drops the session and
rotates at once rather than waiting out `repair_retry_ticks`.
##########
core/shard/src/lib.rs:
##########
@@ -10646,7 +10815,7 @@ fn first_op_not_covered(
.find(|header| header.op == op)
.is_none_or(|canonical| header_is_view_entry(&local, canonical))
};
- (pending.commit_max.max(1).max(repair_floor + 1)..=pending.op_head)
+ (merged_log_scan_floor(pending, commit_min).max(repair_floor +
1)..=pending.op_head)
Review Comment:
Fixed. One `repair_headers_in` pass into a `BTreeMap`, probed by lookup.
Ring-aware too, which is what the widened floor needs.
##########
core/shard/src/lib.rs:
##########
@@ -10646,7 +10815,7 @@ fn first_op_not_covered(
.find(|header| header.op == op)
.is_none_or(|canonical| header_is_view_entry(&local, canonical))
};
- (pending.commit_max.max(1).max(repair_floor + 1)..=pending.op_head)
+ (merged_log_scan_floor(pending, commit_min).max(repair_floor +
1)..=pending.op_head)
.find(|op| !held(*op))
.or_else(|| {
Review Comment:
Fixed your way. `from_op` is stored on the session and the retry floors at
it, so an op the `committed_elsewhere` fallback reports below the scan's open
is re-asked.
##########
core/shard/src/lib.rs:
##########
@@ -5596,51 +5646,44 @@ where
})
};
if let Some((peer, nonce, to_op)) = stalled {
- // A session pins its peer and fences every arming site while it
- // stands, so a peer that cannot answer wedges the plane harder
than
- // having no session at all -- and the gap-stopped-primary rotation
- // can pick a peer that is simply down. Past the budget the session
- // is dropped and re-armed one step around the ring; an ordinary
lost
- // frame is re-requested long before that.
- if self.burn_metadata_repair_attempt() {
- let next_peer = next_transfer_peer(
- consensus.replica(),
- peer,
- consensus.replica_count(),
- consensus.primary_index(consensus.view()),
- );
- tracing::warn!(
+ // Primary-elect only, and floored at `merged_log_scan_floor` so a
+ // retry re-requests the window the initial arm did. The merged
log's
+ // commit point can sit below local `commit_min` (the headers
+ // inherited from senders behind the canonical log_view live
there),
+ // so `commit_min + 1` would skip them, and above it when the local
+ // prefix has a hole, so the commit point alone would skip that. A
+ // backup's parked `StartView` suffix is only a verification
+ // reference; resuming from its commit point would restart at the
+ // view's opening head, not at the gap.
+ let from_op = consensus
+ .is_primary_for_view(consensus.view())
+ .then(|| {
+ consensus.with_pending_view_log(|pending| {
+ merged_log_scan_floor(pending, consensus.commit_min())
+ })
+ })
+ .flatten()
Review Comment:
Fixed by the same change. `session.from_op` carries the initial arm's
`snapshot_op() + 1` clamp, so the retry cannot ask for compacted ops.
##########
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(),
+ |op| journal.inner.header_by_op(op),
Review Comment:
Fixed by stating the assumption. `first_op_not_covered` and the `disagrees`
site now say identity below the merged commit point rests on crash-stop, with
`verify_prepare_integrity` as the corruption guard and neither as a Byzantine
one.
--
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]
