[Issue 8599] Fix DispatchRateLimiter does not take effect

[wangjialing218]

Fixes #8599
Fixes #4777

Motivation

Pulsar current support topic level and subscription level dispatch rate limiter by using DispatchRateLimiter. When consumers connected to broker and start reading entry, broker judge whether rate limit is exceeded before reading, and increasing the permits after reading finished by call tryAcquire(). When there are multi consumers using one DispatchRateLimiter, these consumers could start reading together and may increasing the acquiredPermits far more than permits after reading finished. As acquiredPermits will reset to 0 every second, all consumers could start reading in the next second and dispatch rate limiter will take no effect in such case.

Modifications

This PR change the behaviour of DispatchRateLimiter, minus permits every second instead of reset acquiredPermits to 0, and the reading will stop for a while until acquiredPermits return to a value less than permits .

Verifying this change

RateLimiterTest.testDispatchRate()

[wangjialing218]

/pulsarbot run-failure-checks

[wangjialing218]

/pulsarbot run-failure-checks

[315157973]

If the rate limit is 100 msg/s, and I request 10000 msg concurrently at the same time, 10000 messages will still be read, but in the next 100 seconds, none of the messages will be read. This kind of scenario is very easy to simulate, just set the ReceiveQueueSize of MultiConsumer to be larger. Is this side effect too big?

[wangjialing218]

If the rate limit is 100 msg/s, and I request 10000 msg concurrently at the same time, 10000 messages will still be read, but in the next 100 seconds, none of the messages will be read. This kind of scenario is very easy to simulate, just set the ReceiveQueueSize of MultiConsumer to be larger. Is this side effect too big?

Yes，if we start concurrently read at same time, all conumser could start there first reading and make dispatch rate far over the limiter, this could be another issue and this PR does not aim to solve this.
This PR make none messages read in the next 100 seconds to let the average dispatch rate return to 100 msg/s at last. In some case it is useful. Consider a topic we don't need to dispatch in real time, we could set a low dispatch rate and just keep the data in bookkeeper, left the network resource to other topics.

[wangjialing218]

/pulsarbot run-failure-checks

[wangjialing218]

/pulsarbot run-failure-checks

[wangjialing218]

/pulsarbot run-failure-checks

[jiazhai]

/pulsarbot run-failure-checks

[wangjialing218]

/pulsarbot run-failure-checks

[wangjialing218]

/pulsarbot run-failure-checks

[wangjialing218]

/pulsarbot run-failure-checks

[zymap]

/pulsarbot run-failure-checks

[wangjialing218]

/pulsarbot run-failure-checks

[zymap]

@sijie @codelipenghui @rdhabalia @merlimat Please take a review at this PR. Thanks

[zymap]

push this to the next release.

[wangjialing218]

/pulsarbot run-failure-checks

[eolivelli]

@lhotari FYI

[lhotari]

It seems that with the changes in this PR, the RateLimiter class now has multiple algorithms that is activated with the isDispatchRateLimiter flag. Wouldn't it be better to split 2 separate RateLimiter implementations that implement different algorithms?

In text books, there are algorithms such as leaky bucket and token bucket . Both algorithms have several variations and in some ways they are very similar algorithms but looking from the different point of view. It would possibly be easier to conceptualize and understand a rate limiting algorithm if common algorithm names and implementation choices mentioned in text books would be referenced in the implementation. This is more like an idea if the rate limiter classes get refactored and split as part of some other PRs.

Another problem with the rate limiting are the 2 separate limits: number of messages and number of bytes. The rate limiting seems to behave in the incorrect way if those limits are both hit in some connection that is being rate limited. This is an issue in the "precise topic rate limiting" implementation. There are some comments about the challenge: #10384 (comment) .
Is it also an issue in dispatch rate limiter?

[codelipenghui]

@lhotari Good point, Could you please open a new issue for your last comment? This PR fixes the bug of the current implementation, It's better to use a separate issue to track the new implementation of the RateLimiter.