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 topic dispatch rate limiter not init on broker-level #16084

Merged

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Jun 16, 2022

Motivation

  • Fix dispatch rate limiter doesn't be initialized on broker-level:
    • If DispatchRateLimiter is disabled at first, then we update only broker-level dispatch rate dynamicly, it will not take affect because topic.getDispatchRateLimiter().isPresent() is false, line:2377
      private void updateTopicMessageDispatchRate() {
      this.pulsar().getExecutor().execute(() -> {
      // update message-rate for each topic
      forEachTopic(topic -> {
      if (topic instanceof AbstractTopic) {
      ((AbstractTopic) topic).updateBrokerDispatchRate();
      }
      if (topic.getDispatchRateLimiter().isPresent()) {
      topic.getDispatchRateLimiter().get().updateDispatchRate();
      }

Modifications

  • Create a method updateDispatchRateLimiter() to initialize and update the dispatch rate limiter

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added UT TopicDispatchRateLimiterTest

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 16, 2022
@AnonHxy AnonHxy force-pushed the fix_broker_dispatchratelimiter_init branch from 9e59a9e to 1f383c1 Compare June 16, 2022 05:58
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jun 16, 2022

/pulsarbot run-failure-checks

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jun 18, 2022

/pulsarbot run-failure-checks

@Jason918 Jason918 added type/bug The PR fixed a bug or issue reported a bug release/2.10.2 labels Jun 18, 2022
@Jason918 Jason918 added this to the 2.11.0 milestone Jun 18, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jul 1, 2022

@Technoboy- @codelipenghui PTAL~, thanks

@codelipenghui codelipenghui merged commit 523b5c8 into apache:master Jul 4, 2022
@codelipenghui
Copy link
Contributor

@AnonHxy Could you please help open a PR for branch-2.10 since I'm not able to cherry-pick it directly.

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jul 25, 2022

@AnonHxy Could you please help open a PR for branch-2.10 since I'm not able to cherry-pick it directly.

OK, I am working on this. It is better to merge this cherry-pick first #16778. Then it will be easier to resolve the conflict @codelipenghui

@codelipenghui
Copy link
Contributor

@AnonHxy Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants