New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[issue #13351] Solving precise rate limiting does not takes effect #11446
Conversation
if (permitUpdater != null) { | ||
long newPermitRate = permitUpdater.get(); | ||
if (newPermitRate > 0) { | ||
setRate(newPermitRate); | ||
} | ||
} | ||
if (rateLimitFunction != null) { | ||
if (rateLimitFunction != null && this.getAvailablePermits() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to make sense. Please add some comments here explaining the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd assume that the condition would need to handle the case where isDispatchOrPrecisePublishRateLimiter
is false
.
It seems that rateLimitFunction.apply
would never be called in that case.
Did you think about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea Ill add a comment.
And actually i didn't give it much thought but when I'm thinking about it the rateLimitFunction is a callback that lets the outer scope access to the renew function, I don't think that we should use this property without knowing exactly what expected to be happening.
I believe that checking whether there are available permits is the reasonable condition here because we would want to let the outer scope a way to run something when there are any available permits, and it doesn't really depend on the class property.
If it sets to false we can assume the user of the rateLimiter wants the state to be reset every time window otherwise he probably want back-pressuring something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @danielsinai ! Thanks for the contribution. LGTM.
The failed test actually found a bug that the first tryAcquire will always pass because the condition is running before the acquiredPermits changed, and that will let the producer to write 2x above the limit in the first try. In addition there are some tests that should failed like the test that fills all the permits in the first try - currently it asserting to true but actually we should assert it to false in order to throttle the producer in the first time the maximum permits are set. Ill publish a fix soon |
@@ -104,9 +104,13 @@ public void testPrecisePublishRateLimiterAcquire() throws Exception { | |||
|
|||
// tryAcquire msgSizeInBytes exceeded | |||
assertFalse(precisPublishLimiter.tryAcquire(10, 101)); | |||
Thread.sleep(1100); | |||
Thread.sleep(2100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a potential flaky test, and we cannot guarantee that it will be scheduled after 2100ms. Manual control of renew should be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it, didn't want to touch the current implementation too much.
But will do 👍🏾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like me to turn the rateLimiters within the precisPublishLimiter to public in order to call the renew function?
Im afraid that it can result others to try and access the rateLimiters state, Oppositely to OOP mutable non-shared state approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use reflection.
Stop the scheduled task first
private ScheduledFuture<?> renewTask;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sounds good, will do thanks for clarifying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍🏾
if (permitUpdater != null) { | ||
long newPermitRate = permitUpdater.get(); | ||
if (newPermitRate > 0) { | ||
setRate(newPermitRate); | ||
} | ||
} | ||
if (rateLimitFunction != null) { | ||
// release the back-pressure by applying the rateLimitFunction only when there are available permits | ||
if (rateLimitFunction != null && this.getAvailablePermits() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is necessary
Changed existing behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is necessary, without it every renew call the function will release the throttle.
Not having it defeats the whole purpose of throttling a connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that still relevant?
And may you review the test refactor please?
/pulsarbot run-failure-checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move the code from setup
to unit test so that it will not affect other unit tests
@315157973 yes ofc done 👍🏾 |
Thanks. Let's wait for CI to pass |
@315157973 Seems like everything passed 👌😄 |
…11446) ![image](https://user-images.githubusercontent.com/51213812/126812923-91bb827c-246d-451d-8f25-343bb2c1dca0.png) befoe this PR precise publish rate limiting wasn't taking effect at all ### Modifications In order to solve the current problems, there are 2 modifications 1. Using IsDispatchRateLimiting in precise publish rate limiter as well (in order to starve the producer) 2. Checking if there are available permits before resetting the read from the connection again ### Verifying this change Already covered by current tests. (cherry picked from commit 00ad07d)
…11446) ![image](https://user-images.githubusercontent.com/51213812/126812923-91bb827c-246d-451d-8f25-343bb2c1dca0.png) befoe this PR precise publish rate limiting wasn't taking effect at all In order to solve the current problems, there are 2 modifications 1. Using IsDispatchRateLimiting in precise publish rate limiter as well (in order to starve the producer) 2. Checking if there are available permits before resetting the read from the connection again Already covered by current tests. (cherry picked from commit 00ad07d)
…ect (apache#11446) ![image](https://user-images.githubusercontent.com/51213812/126812923-91bb827c-246d-451d-8f25-343bb2c1dca0.png) befoe this PR precise publish rate limiting wasn't taking effect at all In order to solve the current problems, there are 2 modifications 1. Using IsDispatchRateLimiting in precise publish rate limiter as well (in order to starve the producer) 2. Checking if there are available permits before resetting the read from the connection again Already covered by current tests. (cherry picked from commit 00ad07d) (cherry picked from commit 06c6adf)
…ect (apache#11446) ![image](https://user-images.githubusercontent.com/51213812/126812923-91bb827c-246d-451d-8f25-343bb2c1dca0.png) befoe this PR precise publish rate limiting wasn't taking effect at all ### Modifications In order to solve the current problems, there are 2 modifications 1. Using IsDispatchRateLimiting in precise publish rate limiter as well (in order to starve the producer) 2. Checking if there are available permits before resetting the read from the connection again ### Verifying this change Already covered by current tests.
Master Issue: #11351
Motivation
This PR is fixing the 1,2 problems as described in #11351.
There was a bigger Pull request that I published #11352, but I closed it in due to being too big and lacked explaination for what is actually solves
Reproduce
Allow precise rate limiting
Create a topic
Limit publish rate of messages per second to 10
With producer perf write 100 messages per second
Results after this PR:
befoe this PR precise publishRate limiting wasn't taking effect at all
Modifications
In order to solve the current problems, there are 2 modifications
Verifying this change
Already covered by current tests.
Does this pull request potentially affect one of the following parts:
Documentation
For contributor
For this PR, do we need to update docs? Probably not, it is just fixing the current implementation