rate-limiting: test enhancement + refactoring #223
Conversation
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
…te-limiter Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Modulo an improved test for the batching rate limiter, this pulls off some refactoring for upcoming new rate limiters. Split out off from draft PR envoyproxy#218 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
dubious90
left a comment
dubious90
left a comment
There was a problem hiding this comment.
Mostly looks good. Have a couple questions.
| * Base for a rate limiter which wraps another rate limiter, and forwards | ||
| * some calls. | ||
| */ | ||
| class ForwardingRateLimiterImpl : public RateLimiter { |
There was a problem hiding this comment.
If RateLimiterBaseImpl is the base RateLimiter class that we're working with, should this rate limiter also extend from it? Would it make sense to add a comment explaining why it doesn't? If it's because RateLimiterBaseImpl is too specific to be pulled from here, I might suggest that BaseImpl is a counterintuitive name for it.
There was a problem hiding this comment.
I can see your point, but I have a hard time coming up with a better name; hopefully the comment
I added in 6d1e747 help
There was a problem hiding this comment.
I think that your added comment clarifies things nicely. Thank you
source/common/rate_limiter_impl.cc
Outdated
|
|
||
| bool DelegatingRateLimiter::tryAcquireOne() { | ||
| bool DelegatingRateLimiterImpl::tryAcquireOne() { | ||
| const auto now = timeSource().monotonicTime(); |
There was a problem hiding this comment.
Is there a slight functional change here? I'm not 100% sure I understand how monotonicTime() works here, but because you're pulling this value before tryAcquireOne here, if tryAcquireOne were to ever have a delay for any reason (even if only in certain impls), what now is capturing here is not equivalent to what now was capturing before.
If you don't think this is a legitimate concern, I will completely defer to you on this.
There was a problem hiding this comment.
Nice, catch; I backed this out.
The behavioral change is quite small but as the PR description promised to not introduce these type of changes, this shouldn't go in here - and probably there is no value in making the change at all.
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
LGTM, htuch to approve |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM, just a small docs ask.
/wait
…ctoring Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
3 participants
Modulo an improved test for the batching rate limiter,
this pulls off some refactoring for upcoming new rate
limiters.
This has no functional changes, and is an intermediate step for draft PR #218,
which contains new rate limiters (linear & probabilistic ramping, first stab at a zipf distribution support).
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com