-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][broker] Fix incorrect blockedConsumerOnUnackedMsgs value when maxUnackedMessagesPerConsumer is 1 #23796
base: master
Are you sure you want to change the base?
Conversation
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.
Good catch! I have one question in a comment.
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
Show resolved
Hide resolved
@summeriiii It looks like ZkSessionExpireTest.testTopicUnloadAfterSessionRebuild fails with this change. I haven't checked why, but that test has failed multiple times. |
/pulsarbot run-failure-checks |
…axUnackedMessagesPerConsumer is 1
2f2589e
to
b97df18
Compare
@summeriiii I was able to reproduce the failure by running the test in Docker with the scripts available at https://github.com/lhotari/pulsar-contributor-toolbox/blob/master/functions/pulsar-contributor-toolbox-functions.sh git fetch origin pull/23796/merge
git checkout FETCH_HEAD
ptbx_run_test_in_docker -pl pulsar-broker test -Dtest=ZkSessionExpireTest It's hard to see the reason why it would be caused by this PR. Before the failure, I could see this output
|
previous attempts https://github.com/apache/pulsar/actions/runs/12556467839 . Downloading logs to see if it's similar as what I see locally. |
The test has been flaky for a longer period of time: #23389 |
The same ZkSessionExpireTest.testTopicUnloadAfterSessionRebuild test fails also in branch-4.0 builds. Let's continue addressing it in #23389 . |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #23796 +/- ##
============================================
+ Coverage 73.57% 74.13% +0.56%
+ Complexity 32624 31789 -835
============================================
Files 1877 1853 -24
Lines 139502 143470 +3968
Branches 15299 16291 +992
============================================
+ Hits 102638 106362 +3724
+ Misses 28908 28729 -179
- Partials 7956 8379 +423
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Fixes #22657
Motivation
This issue was introduced by #20990. After we call Consumer#addAndGetUnAckedMsgs to change the unackedMessages, we should check and may update blockedConsumerOnUnackedMsgs.
Modifications
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: