hubcio commented on code in PR #4073:
URL: https://github.com/apache/iggy/pull/4073#discussion_r3955890962
##########
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:
critical: the suggested pop-and-discard here would drop the client reply -
the journal walk applies with `send_client_replies = false`, so nothing has
answered the caller yet. cap both commit walks at `pipeline_head_op - 1`
instead, so the head never falls below `commit_min`.
also at line 528.
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -7845,6 +7845,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() {
Review Comment:
warning: master already covers this at
`given_absent_offset_file_when_delete_commits_should_skip_directory_sync`,
which commits an absent-offset delete through `handle_committed_entries` and
asserts the same thing. the only new coverage here is the `ConsumerGroup` arm -
route that through the same path, or drop the test.
--
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]
