-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix][broker] Key_Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog #24732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…deliver messages from the replay queue after a consumer disconnects and leaves a backlog unless new messages are produced
|
@lhotari FYI: all the checks passed in forked repository nborisov#4 |
lhotari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job @nborisov! One minor detail to address.
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DrainingHashesTracker.java
Outdated
Show resolved
Hide resolved
|
@lhotari Thank you for the review and quick responses! |
lhotari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎯 perfect job, @nborisov
|
btw. Didn't realize until now that this PR got merged into |
|
@nborisov You will need to fork a new branch name from the PR branch (since this PR is now merged) and then rebase it on |
|
@lhotari Sorry for misunderstanding the process! Preparing a new PR based on master branch. |
… from the replay queue after a consumer disconnects and leaves a backlog (apache#24732) Co-authored-by: Nikolai Borisov <nikolai.borisov@onde.app> (cherry picked from commit ca8d338)
… from the replay queue after a consumer disconnects and leaves a backlog (apache#24732) Co-authored-by: Nikolai Borisov <nikolai.borisov@onde.app> (cherry picked from commit ca8d338)
UPDATE: superseded by #24736 to master branch
Fixes #23845
Motivation
There is a not covered scenario for draining hashes and key shared subscriptions at
PersistentStickyKeyDispatcherMultipleConsumers. The detailed scenario described at #23845 (comment)As a result draining hashes could contain entries with consumer which was stopped. This leads consuming to get stuck.
Modifications
Decrease draining hash entry
refCountin case its hash range returned to the initial owner which holds the entry in pending acks.Verifying this change
This change added tests and can be verified as follows:
org.apache.pulsar.client.api.KeySharedSubscriptionTest#testMessageDeliveredFromDrainingHashesto verify the scenarioorg.apache.pulsar.broker.service.ConsistentHashingStickyKeyConsumerSelectorTest#testShouldNotSwapExistingConsumers,org.apache.pulsar.broker.service.ConsumerHashAssignmentsSnapshotTest#testResolveConsumerRemovedHashRanges_NoChanges,org.apache.pulsar.broker.service.ConsumerHashAssignmentsSnapshotTest#testResolveConsumerUpdatedHashRanges_RangeAdded,org.apache.pulsar.broker.service.ConsumerHashAssignmentsSnapshotTest#testResolveConsumerRemovedHashRanges_RangeUpdated,org.apache.pulsar.broker.service.ConsumerHashAssignmentsSnapshotTest#testResolveConsumerUpdatedHashRanges_OverlappingRangesDoes this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: nborisov#4