hubcio commented on code in PR #4500:
URL: https://github.com/apache/iggy/pull/4500#discussion_r4358214040
##########
core/consensus/src/impls.rs:
##########
@@ -4285,7 +4524,11 @@ impl<B: MessageBus, P: Pipeline<Entry = PipelineEntry>>
VsrConsensus<B, P> {
self.view.set(msg_view);
self.log_view.set(msg_view);
self.status.set(Status::Normal);
+ self.rejoined_on_recovered_log.set(false);
self.ceded_primaryship.set(false);
+ // The view's log now comes from a settled primary, and repair fills
this
+ // replica's holes from it.
+ self.untrusted_log.set(false);
Review Comment:
critical: this pre-existing path trusts recovered replicas before body
repair, so a later quorum can discard acknowledged operations. keep them
untrusted until repair completes, and exclude their missing bodies from
negative votes.
##########
core/shard/src/lib.rs:
##########
@@ -9472,6 +9522,60 @@ where
false
}
+ /// Replace a partition whose applied history the view contradicts with a
+ /// fresh copy from its peers, instead of letting the WAL's parent check
+ /// stop the whole node on the view's next prepare.
+ ///
+ /// The rebuild quarantines the local segments as evidence and comes back
+ /// with no materialization, so it serves, acks and votes nothing until a
+ /// state transfer from a settled primary installs the view's history.
+ ///
+ /// `agreed_op` is the highest applied op the view confirms. The recorded
+ /// frontier may sit below the group's but never above it: too high refuses
+ /// every offer on a topic nobody writes to. Falling back to `0` is safe
+ /// because the quarantined partition holds no data to rewind, and an offer
+ /// from a lagging peer is still a consistent prefix of the agreed history.
+ fn resync_diverged_partition(
+ &self,
+ namespace: IggyNamespace,
+ partition: &mut IggyPartition<B, SB>,
+ diverged_op: u64,
+ agreed_op: Option<u64>,
+ ) where
+ B: MessageBus + 'static,
+ T: ShardsTable,
+ {
+ tracing::error!(
+ shard = self.id,
+ namespace_raw = namespace.inner(),
+ diverged_op,
+ view = partition.consensus().view(),
+ commit_min = partition.consensus().commit_min(),
+ "the view contradicts applied partition history at op
{diverged_op}; \
+ quarantining this partition and resyncing it by state transfer"
+ );
+ self.metrics.record_partition_applied_divergence();
+ // The rebuild refuses any offer below the frontier it records, and the
+ // view's history past `agreed_op` can be shorter than this replica's.
+ // Counting this replica's own divergent mints would refuse every offer
+ // until the group wrote past them, which an idle topic never does.
+ let frontier = agreed_op
+ .and_then(|agreed|
partition.offset_frontier_before(agreed.saturating_add(1)))
+ .unwrap_or_else(|| {
+ tracing::warn!(
+ shard = self.id,
+ namespace_raw = namespace.inner(),
+ diverged_op,
+ ?agreed_op,
+ "no retained applied op is confirmed by the view below op
{diverged_op}; \
+ the rebuild records no offset frontier and accepts any
offer"
+ );
+ 0
+ });
+ self.drop_partition_transfer_state(namespace, partition);
+ self.fence_partition_for_rebuild(namespace, partition, frontier);
Review Comment:
warning: resync keeps divergent WAL and consumer offsets, so reload can
reject valid canonical transfers. move those files aside after their writers
drain and before reload.
##########
core/shard/src/lib.rs:
##########
@@ -5150,14 +5152,48 @@ where
return;
};
partition.ensure_materialization_recovery();
- let Some(actions) = partition.consensus().handle_start_view_guarded(
+ let actions = match partition.consensus().try_handle_start_view(
PlaneKind::Partitions,
&header,
suffix_body,
|op| partition.applied_header(op),
- ) else {
- return;
+ ) {
+ Ok(actions) => actions,
+ // Refusing alone leaves this replica refusing every retransmit of
the
+ // view, out of the group for good. The refused log still names the
+ // history both sides share, which is where the rebuild resumes.
+ Err(StartViewRefusal::ContradictsAppliedLog(refused)) => {
+ if let Some((diverged_op, agreed_op)) =
+ partition_applied_divergence(partition, &refused)
+ {
+ self.resync_diverged_partition(
+ IggyNamespace::from_raw(header.group),
+ partition,
+ diverged_op,
+ agreed_op,
+ );
+ }
+ return;
+ }
+ Err(StartViewRefusal::InvalidEvidence) => return,
};
+ // Checked on every adoption, transferring or not: a transfer that
later
+ // fails leaves the applied history the view contradicts in place.
+ if !actions.is_empty()
+ && let Some((diverged_op, agreed_op)) = partition
+ .consensus()
+ .pending_view_log()
Review Comment:
simplification: `on_start_view` clones the header vectors only for a
synchronous divergence check. use `with_pending_view_log` and return the result
before quarantine.
--
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]
