ronfarkash opened a new pull request #11372: URL: https://github.com/apache/pulsar/pull/11372
## Master Issue: <https://github.com/apache/pulsar/issues/11351> ### Motivation Hello, as far as I'm concerned it is well known that precise publish rate limiting does not function well. I believe my PR fixes problem number 3 stated in the issue above. @danielsinai: "3. Rate limit function passed only to the msg/s rate limiter (and that's in order to avoid calling it twice)" It was passed to message rate limiter only due to the fact that there was no implementation of a way to throttle the connection whenever only **one of the limiters was exceeded**. This PR will allow both message rate & byte rate to co-exist, limit and enable socket reading only when necessary. ### Modifications - _tryAcquire_ function in **PublishRateLimiterDisable** will return true. If publish rate was null, this function would get called and return false, thus throttling the client for no reason. If the publish rate is null, it means it was not set by anyone so there's no reason to throttle any connection. ```java public boolean tryAcquire(int numbers, long bytes) { return true; } ``` - **RateLimiter** _permits_ and _acquiredPermits_ were changed to volatile. ```java private volatile long permits; private volatile long acquiredPermits; ``` in order to allow reading access from multiple threads at the same time. also the removal of _synchronized_ keyword from _getAvailablePermits()_ function. ```java public long getAvailablePermits() { return Math.max(0, this.permits - this.acquiredPermits); } ``` - Created a HashMap to manage the byte and message rate limiters, and a function _releaseThrottle()_ to handle the auto read enable. If one of the rate limiters has no available permits we will not re-enable the auto read from the socket. ```java private void releaseThrottle() { for (RateLimiter rateLimiter : rateLimiters.values()) { if (rateLimiter.getAvailablePermits() <= 0) { return; } } this.rateLimitFunction.apply(); } ``` ### Verifying this change This change is already covered by existing tests, such as PrecisRateLimiterTest. ### Does this pull request potentially affect one of the following parts: *If `yes` was chosen, please highlight the changes* - Dependencies (does it add or upgrade a dependency): no - The public API: no - The schema: no - The default values of configurations: no - The wire protocol: no - The rest endpoints: no - The admin cli options: no - Anything that affects deployment: no ### Documentation #### For contributor For this PR, do we need to update docs? No, this PR fixes bugs of existing documented features. ## Important Additional Information This PR fixes some core issues with precise publish rate limiting but is depdenent on another PR #11352 , I would highly prefer @danielsinai PR to be merged first before this one since it fixes **core issues** regarding publish rate limiting and in order to prevent unnecessary disfunctionallities. @lhotari also has a PR in the works fixing other issues related to the same topic #10384. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
