[fix][consumer] Initialize batch ack tracker from the broker ack set on redelivery#1521
Open
Shawyeok wants to merge 1 commit into
Open
[fix][consumer] Initialize batch ack tracker from the broker ack set on redelivery#1521Shawyeok wants to merge 1 commit into
Shawyeok wants to merge 1 commit into
Conversation
…on redelivery When the broker redelivers a partially-acked batch entry, it attaches the ack set of the already-acked indexes. The client skipped delivering those indexes but initialized the batch ackTracker as if all of them were still outstanding. For a consumer without batch index ack, whose only way to ack a batch is the entry-level ack gated on the tracker completing, acks of the redelivered messages were silently dropped, so the batch was never acked at the broker and its messages could end up in the DLQ despite having been acked in time. Seed the tracker from the broker-provided ack set when one is present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This ports the fix from apache/pulsar#26135 ([fix][client] Sync ackSet in client with broker to stop acked messages reaching the DLQ) to the Go client.
When the broker redelivers a batch entry that was partially acked with batch index acknowledgment, it attaches the ack set of the already-acked indexes to the
CommandMessage. The Go client uses that ack set to skip delivering the acked indexes, but initializes the batch's localackTrackeras if every index were still outstanding.For a consumer without batch index ack this is fatal: such a consumer can only ack a batch as a whole entry, once the tracker reports every index acked. A stale tracker can never complete — the broker-acked indexes are never delivered again, so their bits are never cleared — and
ackIDCommonsilently drops the acks of the redelivered messages. The batch is never acked at the broker and its messages are redelivered to the next consumer session; with a dead letter policy their redeliveries eventually exhaust, and messages the application acked in time end up in the DLQ.(For a consumer with batch index ack the stale tracker is harmless: each ack is sent as an individual batch-index ack and the broker intersects ack sets, so the broker state converges regardless. This is why the regression test below runs the final-round acks through a consumer without batch index ack.)
Modifications
newAckTrackertakes the set of outstanding batch indexes as a parameter, and the consumer seeds it with the broker-provided ack set of the redelivered batch when one is present (nilkeeps the previous all-outstanding behavior).TestAckTrackerWithBatchIDs, dedicated unit tests for the amendednewAckTracker: seeding from a broker ack set, completion by acking only the outstanding indexes (individually and cumulatively), clone semantics of the passed bitset, and thenilcontract.TestAckedBatchMessageNotSentToDeadLetterTopicWithoutBatchIndexAck, a Go adaptation oftestAckedBatchMessageNotSentToDeadLetterTopicOnFinalRedeliveryRoundfrom the original PR: batch indexes 1 and 2 of a partially-acked batch are nacked until their final allowed redelivery round, acked there, and the test asserts that nothing is routed to the DLQ. Two Go-specific adaptations: redelivery is driven byNack(the Go client has no ackTimeout), and the final application-visible round isredeliveryCount == MaxDeliveries-1, since the Go client routes messages that reachMaxDeliveriesstraight to the DLQ router without delivering them to the application.Verifying this change
This change added tests and can be verified as follows:
Against a local Pulsar standalone (
apachepulsar/pulsar:4.0.10withintegration-tests/conf/standalone.conf):passes with the fix. With the fix reverted (
pulsar/consumer_partition.gofrom master), the test fails — messages 1 and 2, acked on their final redelivery round, are routed to the DLQ:go test ./pulsar -run '^TestBatchIndexAck$' -count=1still passes.Unit tests:
go test ./pulsar -run '^TestAckTracker$|^TestAckTrackerWithBatchIDs$|^TestAckingMessageIDBatchOne$|^TestAckingMessageIDBatchTwo$' -count=1(no broker required).Does this pull request potentially affect one of the following parts:
Documentation
🤖 Generated with Claude Code