[GitHub] [pulsar] lhotari commented on pull request #8611: [Issue 8599] Fix DispatchRateLimiter does not take effect

[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