You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/11/18 09:12:16 UTC
[GitHub] [pulsar] wangjialing218 opened a new pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
wangjialing218 opened a new pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611
Fixes #8599
### 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()
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-730201926
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-730822613
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-827393661
@lhotari FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-730802715
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-733612112
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-730203775
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-845828963
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](https://en.wikipedia.org/wiki/Leaky_bucket) and [token bucket](https://en.wikipedia.org/wiki/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: https://github.com/apache/pulsar/pull/10384#issuecomment-842292133 .
Is it also an issue in dispatch rate limiter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui edited a comment on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
codelipenghui edited a comment on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-845923286
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-785514585
push this to the next release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-789447876
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-845923286
@lhotari 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-784183346
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-734148361
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-734190179
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-784747255
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-730368234
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap edited a comment on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
zymap edited a comment on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-785073814
@sijie @codelipenghui @rdhabalia @merlimat Please take a review at this PR. Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-732909708
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-785073814
@sijie @codelipenghui Please take a review at this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-733089116
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #8611:
URL: https://github.com/apache/pulsar/pull/8611#issuecomment-783861347
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org