Fix precise publish rate limiting #11351

danielsinai · 2021-07-17T21:13:57Z

Is your enhancement request related to a problem? Please describe.
Currently, precise publish rate limiting doesn't work as expected and that's because multiple reasons

Renew function called every second and resets reading from socket every second no matter how many messages produced in the last second (FixedWindow Algorithm)
The FixedWindow algorithm does not fit here because a permit is not 1:1 (multiple messages in batch, bytes per second)
Rate limit function passed only to the msg/s rate limiter (and that's in order to avoid calling it twice)
The first message of any new producer that connects to the broker will get in

Describe the solution you'd like
1 + 2. in order to fix that, I believe we should implement another RateLimiter using the LeakingBucket Algorithm along with FixedWindow
3. we should pass a rate limit function that depends on the state of both of the rate limiters
4. I don't really have an idea how to implement a fix for it would love to hear ur opinions

Additional context
The main idea is thanks to #8611 (comment) @lhotari

danielsinai · 2021-07-17T21:15:11Z

I will publish a pull request soon

@danielsinai

## Master Issue: <#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); } ``` **This is required, since a thread dead lock will happen if not.** - 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.

@danielsinai

## Master Issue: <#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); } ``` **This is required, since a thread dead lock will happen if not.** - 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. (cherry picked from commit 7f2ca8f)

@danielsinai

## Master Issue: <#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); } ``` **This is required, since a thread dead lock will happen if not.** - 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. (cherry picked from commit 7f2ca8f)

@danielsinai

…he#11372) 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. - _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); } ``` **This is required, since a thread dead lock will happen if not.** - 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. (cherry picked from commit 7f2ca8f) (cherry picked from commit ab5fb72)

codelipenghui · 2022-03-04T06:56:46Z

The issue had no activity for 30 days, mark with Stale label.

@danielsinai

…he#11372) ## Master Issue: <apache#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); } ``` **This is required, since a thread dead lock will happen if not.** - 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.

lhotari · 2022-08-16T17:06:49Z

This issue was fixed by #11446 and #11372

danielsinai added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Jul 17, 2021

danielsinai mentioned this issue Jul 17, 2021

[ISSUE #11351] separate Rate Limiter Implementation to LeakyBucket and FixedWindow #11352

Closed

sijie mentioned this issue Jul 17, 2021

ISSUE-11351: Fix precise publish rate limiting streamnative/pulsar-archived#2788

Open

ronfarkash mentioned this issue Jul 19, 2021

[Issue #11351] Parallel Precise Publish Rate Limiting Fix #11372

Merged

danielsinai mentioned this issue Jul 23, 2021

[issue #13351] Solving precise rate limiting does not takes effect #11446

Merged

codelipenghui added the lifecycle/stale label Mar 4, 2022

lhotari closed this as completed Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix precise publish rate limiting #11351

Fix precise publish rate limiting #11351

danielsinai commented Jul 17, 2021

danielsinai commented Jul 17, 2021

codelipenghui commented Mar 4, 2022

lhotari commented Aug 16, 2022 •

edited

Fix precise publish rate limiting #11351

Fix precise publish rate limiting #11351

Comments

danielsinai commented Jul 17, 2021

danielsinai commented Jul 17, 2021

codelipenghui commented Mar 4, 2022

lhotari commented Aug 16, 2022 • edited

lhotari commented Aug 16, 2022 •

edited