-
Notifications
You must be signed in to change notification settings - Fork 82
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
Implement concurrency limit template #32
base: master
Are you sure you want to change the base?
Conversation
@masim05 my only practical question is that if you want to limit concurrency without limiting RPS why not just use a high enough number for RPS? The other thing I was thinking about is that modifying the existing template in a way that zero RPS means no RPS limiting would be better than having multiple templates doing almost the same thing. |
This is exactly the point: if you use big number as
Sounds reasonable. |
@hexdigest |
@@ -63,6 +63,7 @@ If the file is not found, gowrap will look for the template [here](https://githu | |||
|
|||
List of available templates: | |||
- [circuitbreaker](https://github.com/hexdigest/gowrap/tree/master/templates/circuitbreaker) stops executing methods of the wrapped interface after the specified number of consecutive errors and resumes execution after the specified delay | |||
- [concurrencylimit](https://github.com/hexdigest/gowrap/tree/master/templates/concurrencylimit) limits amount of simultaneous calls |
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.
"limits THE amount" please
@masim05 when I got back to you PR, ran tests and look at the original ratelimit template I realized that there was no intention to limit concurrency with this decorator. The problem is that concurrentRequests parameter has a misleading name. I renamed it to "burst". This is the amount of requests that the source interface can handle during the second/rps interval. |
Sometimes it is required to limit concurrency without RPS limitation.
Let's consider a situation we have a function which takes 100 ms and we want to have 3 or less calls of the function at any moment. We may use
ratelimit
template with parameters{concurrentRequests: 3, rps: 30}
.Let's consider a situation we have a function which takes 1 s and we want to have 3 or less calls of the function at any moment. We may use
ratelimit
template with parameters{concurrentRequests: 3, rps: 3}
.The problem here is that
rps
parameter depends on the time of the function's execution which is usually unknown and is not a constant.In other words if we don't want to limit RPS an interface instrumented with
ratelimit
template doesn't limit concurrency. Take a look at https://github.com/masim05/gowrap/blob/concurrency-limit-motivation/templates_tests/interface_with_ratelimit_test.go#L37 (which fails) for more details.Since the change will be backward incompatible with current implementation of
ratelimit
template (in terms of runtime behaviour) it is worth having a separate template for concurrency limitation. This PR provides the implementation.