Skip to content
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 the wrong value of BrokerSrevice.maxUnackedMsgsPerDispatcher #21765

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

aloyszhang
Copy link
Contributor

@aloyszhang aloyszhang commented Dec 20, 2023

Motivation

Pulsar has the limit of max unacked messages at

  • broker level
  • subscription level
  • consumer level

In the product environment(maxUnackedMessagesPerBroker > 0), we met the subscription blocked unexpectedly even if we have set the limit of max un-ack for subscription and consumer big enough.

After troubleshooting, this is caused by the wrong default value of maxUnackedMessagesPerSubscriptionOnBrokerBlocked which is 0.16 now.

For example, if set maxUnackedMessagesPerBroker to 60000, the BrokerService#maxUnackedMsgsPerDispatcher should be 60000 * 1/6 = 10000 by default, but now it's 600

 this.maxUnackedMessages = pulsar.getConfiguration().getMaxUnackedMessagesPerBroker();
            this.maxUnackedMsgsPerDispatcher = maxUnackedMessages
                    * pulsar.getConfiguration().getMaxUnackedMessagesPerSubscriptionOnBrokerBlocked() / 100;

Modifications

  • correct the value of maxUnackedMsgsPerDispatcher
  • modify the related tests

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

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

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

aloyszhang#22

Copy link

@aloyszhang Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Dec 20, 2023
@aloyszhang aloyszhang changed the title fix the wrong type and default value of maxUnackedMessagesPerSubscriptionOnBrokerBlocked [fix][broker] fix the wrong type and default value of maxUnackedMessagesPerSubscriptionOnBrokerBlocked Dec 20, 2023
@Technoboy- Technoboy- added this to the 3.2.0 milestone Dec 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69a45a1) 73.43% compared to head (7c0de10) 73.54%.
Report is 26 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21765      +/-   ##
============================================
+ Coverage     73.43%   73.54%   +0.10%     
+ Complexity    32798    32288     -510     
============================================
  Files          1897     1858      -39     
  Lines        140647   138146    -2501     
  Branches      15489    15141     -348     
============================================
- Hits         103290   101605    -1685     
+ Misses        29283    28674     -609     
+ Partials       8074     7867     -207     
Flag Coverage Δ
inttests 24.16% <0.00%> (+<0.01%) ⬆️
systests 23.71% <0.00%> (-1.05%) ⬇️
unittests 72.84% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rg/apache/pulsar/broker/service/BrokerService.java 80.99% <100.00%> (-0.16%) ⬇️

... and 133 files with indirect coverage changes

@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@aloyszhang
Copy link
Contributor Author

aloyszhang commented Dec 27, 2023

@Technoboy- @codelipenghui @poorbarcode
PTAL, thanks

@aloyszhang
Copy link
Contributor Author

@315157973 @Technoboy- Apply the comment, PTAL again.

@aloyszhang aloyszhang changed the title [fix][broker] fix the wrong type and default value of maxUnackedMessagesPerSubscriptionOnBrokerBlocked [fix][broker] fix the wrong value of BrokerSrevice.maxUnackedMsgsPerDispatcher Jan 5, 2024
@codelipenghui codelipenghui merged commit 67eb3c4 into apache:master Jan 5, 2024
53 checks passed
@Technoboy- Technoboy- modified the milestones: 3.3.0, 3.2.0 Jan 5, 2024
@Technoboy- Technoboy- changed the title [fix][broker] fix the wrong value of BrokerSrevice.maxUnackedMsgsPerDispatcher [fix][broker] Fix the wrong value of BrokerSrevice.maxUnackedMsgsPerDispatcher Jan 8, 2024
nodece pushed a commit to nodece/pulsar that referenced this pull request Feb 23, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants