gowrap
gowrap copied to clipboard
hexdigest
Implement concurrency limit template
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.
Pull Request Test Coverage Report for Build 107
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage remained the same at 96.639%
| Totals | |
|---|---|
| Change from base Build 106: | 0.0% |
| Covered Lines: | 230 |
| Relevant Lines: | 238 |
💛 - Coveralls
@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.
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?
This is exactly the point: if you use big number as rps parameter and every call lasts long enough, you may end up in a situation when many calls are running simultaneously as shown here. In other words, concurrency is not limited.
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.
Sounds reasonable.
@hexdigest It took me some time to realize that rate limiting and concurrency limiting are not the same. Rate limit does not limit concurrency if call executions last long enough. Concurrency limit does not guarantee rate limit if call executions are fast enough. Don't you think it would be nice to have two independent templates and thus follow unix philosophy? =)
@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.