[fix][broker] Shared subscription stalls forever on a chunked message - #26396
Open
0asys wants to merge 1 commit into
Open
[fix][broker] Shared subscription stalls forever on a chunked message#263960asys wants to merge 1 commit into
0asys wants to merge 1 commit into
Conversation
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.
Fixes #26395
Motivation
A Shared subscription stops dispatching when it runs into a chunk of a chunked message whose chunk 0 has already been acknowledged.
SharedConsumerAssignor pins all chunks of a message to one consumer through the in-memory uuidToConsumer map, which is only written when chunk 0 is assigned. If the mapping is gone by the time a later chunk is dispatched (consumer restart, topic unload, broker restart), it hits:
if (metadata.getChunkId() != 0) {
// Not the first chunk, skip it
return null;
}
Modifications
SharedConsumerAssignor:chunkId != 0and no cached mapping is now dispatched to the default consumerinstead of being returned as unassignable. The mapping is only ever seeded by chunk 0, and the
cursor always re-reads chunk 0 ahead of later chunks while it is still unacked, so a missing
mapping means chunk 0 is already acked and the message can never be reassembled. Such an entry
can never become assignable, so replaying it stalls the subscription, while dispatching it lets
the client discard the incomplete chunked message and move on. Logged at WARN, the old path was
completely silent.
removeConsumer(Consumer)andclear()to drop stale uuid mappings.PersistentDispatcherMultipleConsumersandPersistentDispatcherMultipleConsumersClassic:assignor.removeConsumer()inremoveConsumer()andassignor.clear()inclearComponentsAfterRemovedAllConsumers(). BothPersistentStickyKeyDispatcherMultipleConsumersvariants call
super.removeConsumer(), so Key_Shared gets the fix as well.The broker deliberately does not acknowledge the orphan chunk. Dropping an incomplete chunked
message is the client's decision (
autoAckOldestChunkedMessageOnQueueFull,expireTimeOfIncompleteChunkedMessage,AutoAckIncompleteChunkin the Go client), the broker'sjob here is just to stop refusing to deliver it. With auto-ack disabled on the client the entry
stays unacked and gets redelivered and discarded on every reconnect, but the subscription keeps
making progress.
Verifying this change
This change added tests and can be verified as follows:
SharedConsumerAssignorTest.testOrphanChunkIsDispatchedInsteadOfReplayedForevercovers theactual bug: a chunk with
chunkId != 0and an empty uuid cache must be dispatched and must notend up in the replay queue, for a last chunk as well as a middle one. It fails on the current
code with
lists don't have the same size expected [1] but found [0], since the entry isreplayed instead of dispatched.
SharedConsumerAssignorTest.testUuidMappingIsRemovedWhenConsumerIsRemovedcovers dropping themapping of a removed consumer and dispatching the remaining chunk to another one.
SharedConsumerAssignorTest.testClearRemovesAllUuidMappingscovers the leak.The last two exercise methods that do not exist before this change.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes