[Issue #11351] Parallel Precise Publish Rate Limiting Fix #11372
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ronfarkash
changed the title
ronfarkash
changed the title
lhotari
requested changes
Jul 23, 2021
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PrecisPublishLimiter.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PublishRateLimiterDisable.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/util/RateLimiter.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PrecisPublishLimiter.java
Outdated
Show resolved
Hide resolved
lhotari
reviewed
Jul 23, 2021
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PrecisPublishLimiter.java
Show resolved
Hide resolved
lhotari
approved these changes
Jul 23, 2021
|
/pulsarbot run-failure-checks |
|
It seems that every time I run it, other tests fail. I believe my changes didn't hurt anything since the tests that are failing are not related to my changes. |
Yes, there are quite a few flaky tests. Please report the flaky tests as GitHub issues up unless they have already been reported. |
sijie
approved these changes
Jul 28, 2021
codelipenghui
approved these changes
Jul 28, 2021
|
|
Is there anything else need to be done, does this test prevent this PR from being merged even though it's unrelated to my changes? |
codelipenghui
pushed a commit
that referenced
this pull request
Jul 30, 2021
## 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)
michaeljmarshall
pushed a commit
that referenced
this pull request
Dec 10, 2021
## 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)
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Jan 26, 2022
…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)
bharanic-dev
pushed a commit
to bharanic-dev/pulsar
that referenced
this pull request
Mar 18, 2022
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
in order to allow reading access from multiple threads at the same time.
also the removal of synchronized keyword from getAvailablePermits() function.
This is required, since a thread dead lock will happen if not.
a HashMap to manage the byte and message rate limiters, anda 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.
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
yeswas chosen, please highlight the changesDocumentation
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 #11446 , 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.