Skip to content

Conversation

@wangjialing218
Copy link
Contributor

Motivation

Fixes #7720

Current we support broker level publish rate limiter.
This PR will add broker level dispatch rate limiter.

Modifications

Add broker level dispatch rate limiter for msgs and bytes.
Refact calculateToRead() to reduce duplicate code.

Verifying this change

Add testBrokerBytesRateLimitingReceiveAllMessagesAfterThrottling()

@wangjialing218
Copy link
Contributor Author

@codelipenghui @315157973 please have a look

@Anonymitaet Anonymitaet added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jul 16, 2021
@Anonymitaet
Copy link
Member

Thanks for your contribution.

The PR template contains info about doc, which helps others know more about the changes. Can you provide doc info in future PR descriptions? Thanks

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wangjialing218
Copy link
Contributor Author

@315157973 @codelipenghui @hangc0276 PTAL when you have time

@315157973
Copy link
Contributor

The current reading process is like this:

  1. Determine whether the threshold is exceeded
  2. Enter the pending queue and prepare to read
  3. The entry is successfully read, and the threshold count is deducted

There is a time difference between 1 and 3. When the Broker just restarts, if it is read concurrently, the problem in that PR will still exists

@wangjialing218
Copy link
Contributor Author

The current reading process is like this:

  1. Determine whether the threshold is exceeded
  2. Enter the pending queue and prepare to read
  3. The entry is successfully read, and the threshold count is deducted

There is a time difference between 1 and 3. When the Broker just restarts, if it is read concurrently, the problem in that PR will still exists

The problem you mentioned may be solved in this PR #8611

@315157973
Copy link
Contributor

The current reading process is like this:

  1. Determine whether the threshold is exceeded
  2. Enter the pending queue and prepare to read
  3. The entry is successfully read, and the threshold count is deducted

There is a time difference between 1 and 3. When the Broker just restarts, if it is read concurrently, the problem in that PR will still exists

The problem you mentioned may be solved in this PR #8611

This PR will only prevent the message from being sent to the consumer for a long time later, but it cannot solve the memory surge during concurrent reading when broker restart.

@315157973
Copy link
Contributor

We can support broker level dispatcher rate limiter, but this PR can not fix #7720

@wangjialing218
Copy link
Contributor Author

This PR will only prevent the message from being sent to the consumer for a long time later, but it cannot solve the memory surge during concurrent reading when broker restart.

As current reading process, it's not easy to completely prevent the sudden increase entry read traffic when mutliply comsumers starting at same time.
With PR #8611 and this PR, the increase traffic will last only one second. This could reduce the influence of write traffic in case of #7720


long start = System.currentTimeMillis();
// Asynchronously produce messages
for (int i = 0; i < numProducedMessagesEachTopic; i++) {
Copy link
Contributor

@315157973 315157973 Oct 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to stop the thread pool that refreshes the quota in rateLimiter at first, otherwise this unit test will often fail because we cannot predict the running of the thread

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point out the thread which need to stop?

consumer2.close();
producer1.close();
producer2.close();
log.info("-- Exiting {} test --", methodName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test that test the priority of policies at different levels

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For topic dispatch rate limiter, we can configure from namespace level policies and topic level policies, and topic level policies will have a high priority. That's because the enforcement is done at the topic level.
For broker level, topic level and subscription level dispatch rate limiter, the enforcement is done at different level, so they have same priority and take effect together.
I have add a test, when all these dispatch rate limiter are configured, the minimum dispatch rate limiter should take effect in case of one consumer for one topic.

@wangjialing218
Copy link
Contributor Author

@315157973 could you have a look again

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@315157973
Copy link
Contributor

Some unit tests seem to fail

wangjialing and others added 6 commits October 12, 2021 15:11
…riptionMessageDispatchThrottlingTest.java

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
…riptionMessageDispatchThrottlingTest.java

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
@wangjialing218 wangjialing218 force-pushed the branch-broker-dispatch-rate-limiter branch from 544c69b to 9d51e98 Compare October 12, 2021 07:11
wangjialing and others added 6 commits October 15, 2021 10:29
…riptionMessageDispatchThrottlingTest.java

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
…riptionMessageDispatchThrottlingTest.java

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
@wangjialing218 wangjialing218 force-pushed the branch-broker-dispatch-rate-limiter branch from 5f758c9 to b7be33e Compare October 15, 2021 04:50
@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui added this to the 2.10.0 milestone Nov 10, 2021
conf/broker.conf Outdated

# Max Rate(in 1 seconds) of Message allowed to dispatch from a broker if broker dispatch rate limiting enabled
# (Disable message rate limit with value 0)
brokerDispatchThrottlingMaxMessageRate=0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
brokerDispatchThrottlingMaxMessageRate=0
dispatchThrottlingRateInMsg=0

@codelipenghui codelipenghui added doc-required Your PR changes impact docs and you will update later. and removed doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels Jan 18, 2022
@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit 497e911 into apache:master Jan 18, 2022
@Anonymitaet
Copy link
Member

As discussed w/ @wangjialing218, he will add the two parameters and corresponding descriptions to the configuration - standalone page.

@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-complete Your PR changes impact docs and the related docs have been already added.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broker level dispatch rate limiter

4 participants