You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nborisov (via GitHub)" <gi...@apache.org> on 2025/09/11 10:30:05 UTC
[PR] [fix][broker] Key_Shared or Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog unless new messages are produced [pulsar]
nborisov opened a new pull request, #24732:
URL: https://github.com/apache/pulsar/pull/24732
Fixes https://github.com/apache/pulsar/issues/23845
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
### Motivation
There is a not covered scenario for draining hashes and key shared subscriptions at `PersistentStickyKeyDispatcherMultipleConsumers`. The detailed scenario described at https://github.com/apache/pulsar/issues/23845#issuecomment-3269364004
As a result draining hashes could contain entries with consumer which was stopped. This leads consuming to get stuck.
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
Decrease draining hash entry `refCount` in case its hash range returned to the initial owner which holds the entry in pending acks.
<!-- Describe the modifications you've done. -->
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- *Added integration test `org.apache.pulsar.client.api.KeySharedSubscriptionTest#testMessageDeliveredFromDrainingHashes` to verify the scenario*
- Existing unit tests were modified to check changes applied in PR: `org.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_OverlappingRanges`
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared or Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog unless new messages are produced [pulsar]
Posted by "nborisov (via GitHub)" <gi...@apache.org>.
nborisov commented on PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#issuecomment-3284076082
@lhotari Thank you for the review and quick responses!
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #24732:
URL: https://github.com/apache/pulsar/pull/24732
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared or Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog unless new messages are produced [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#discussion_r2342144987
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DrainingHashesTracker.java:
##########
@@ -397,6 +397,12 @@ public boolean shouldBlockStickyKeyHash(Consumer consumer, int stickyKeyHash) {
lock.writeLock().lock();
try {
drainingHashes.remove(stickyKeyHash, entry);
+ // update the consumer specific stats
+ ConsumerDrainingHashesStats drainingHashesStats =
+ consumerDrainingHashesStatsMap.get(new ConsumerIdentityWrapper(consumer));
+ if (drainingHashesStats != null) {
+ drainingHashesStats.clearHash(stickyKeyHash);
+ }
Review Comment:
It would be better to handle this after the writeLock has been release so that possible deadlocks are avoided. That has been a bug pattern in the past.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#issuecomment-3284524875
btw. Didn't realize until now that this PR got merged into `branch-4.0`. @nborisov Please open a new PR with the same content to `master` branch since in Pulsar, the process goes from the master branch and committers handle cherry-picking and backporting and ask for backports when needed. I'll revert this from branch-4.0 and let's start from `master` branch.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog [pulsar]
Posted by "nborisov (via GitHub)" <gi...@apache.org>.
nborisov commented on PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#issuecomment-3284669076
@lhotari Sorry for misunderstanding the process! Preparing a new PR based on master branch.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared or Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog unless new messages are produced [pulsar]
Posted by "nborisov (via GitHub)" <gi...@apache.org>.
nborisov commented on PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#issuecomment-3281367737
@lhotari FYI: all the checks passed at the forked repository https://github.com/nborisov/pulsar/pull/4
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog [pulsar]
Posted by "nborisov (via GitHub)" <gi...@apache.org>.
nborisov commented on PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#issuecomment-3284753768
@lhotari Created the new PR https://github.com/apache/pulsar/pull/24736. Please let me know if I it following the process or I need to make some changes.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared or Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog unless new messages are produced [pulsar]
Posted by "nborisov (via GitHub)" <gi...@apache.org>.
nborisov commented on code in PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#discussion_r2343241930
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DrainingHashesTracker.java:
##########
@@ -397,6 +397,12 @@ public boolean shouldBlockStickyKeyHash(Consumer consumer, int stickyKeyHash) {
lock.writeLock().lock();
try {
drainingHashes.remove(stickyKeyHash, entry);
+ // update the consumer specific stats
+ ConsumerDrainingHashesStats drainingHashesStats =
+ consumerDrainingHashesStatsMap.get(new ConsumerIdentityWrapper(consumer));
+ if (drainingHashesStats != null) {
+ drainingHashesStats.clearHash(stickyKeyHash);
+ }
Review Comment:
Good point, thank you! Fixed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Key_Shared subscription doesn't always deliver messages from the replay queue after a consumer disconnects and leaves a backlog [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24732:
URL: https://github.com/apache/pulsar/pull/24732#issuecomment-3284544937
@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 `origin/master` to get the PR commits there. I'd assume that the changes apply cleanly in rebasing. I'm sorry for the hassle, I didn't notice the target branch in merging. Before merging, it would have been possible to update the target branch.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org