Bucket: expose Available() and Capacity() #10
Conversation
| @@ -171,6 +171,16 @@ func (tb *Bucket) takeAvailable(now time.Time, count int64) int64 { | |||
| return count | |||
| } | |||
|
|
|||
| // Available returns the number of available tokens | |||
| func (tb *Bucket) Available() int64 { | |||
| return tb.avail | |||
There was a problem hiding this comment.
tb.mu should be held while accessing tb.avail
|
Seems fine to me, with one small fix; deferring to @rogpeppe, the author, though. |
| @@ -171,6 +171,18 @@ func (tb *Bucket) takeAvailable(now time.Time, count int64) int64 { | |||
| return count | |||
| } | |||
|
|
|||
| // Available returns the number of available tokens | |||
| func (tb *Bucket) Available() int64 { | |||
There was a problem hiding this comment.
I guess we'd better also doc that It will be negative when there are consumers waiting for tokens.
|
Thanks for this. Please rebase to a single commit before re-proposing, thanks. |
|
Thanks @rogpeppe for your reply. I realized my mistake and fixed it. I squash all I want to do in single commit. However, I add another commit to update Let me know if there are questions. Thanks again! |
|
@rogpeppe |
hongchaodeng
force-pushed
the
master
branch
2 times, most recently
from
e65d914 to
99e1293
Compare
|
@rogpeppe |
|
|
||
| expectCountAfterTake int64 | ||
| expectCountAfterSleep int64 | ||
| }{ |
There was a problem hiding this comment.
}{{
to save an indent level.
Also, please move the tests to just before TestAvailable.
|
Thanks very much for making these changes. LGTM modulo some comments above. |
| sleep: time.Second, | ||
| expectCountAfterTake: 1, | ||
| expectCountAfterSleep: 2, | ||
| }, { // shouldn't fill before interval |
There was a problem hiding this comment.
@rogpeppe
Add two more tests to check boundaries. PTAL.
|
LGTM with one trivial suggestion. |
| expectCountAfterTake int64 | ||
| expectCountAfterSleep int64 | ||
| }{{ | ||
| about: "should fill tokens after interval", |
There was a problem hiding this comment.
@rogpeppe
Added about in table and test failure output.
| @@ -171,6 +171,28 @@ func (tb *Bucket) takeAvailable(now time.Time, count int64) int64 { | |||
| return count | |||
| } | |||
|
|
|||
| // Available returns the number of available tokens. | |||
| // It will be negative when there are consumers waiting for tokens. | |||
| // Note that this method is inherently racy - the returned count | |||
There was a problem hiding this comment.
rather than "inherently racy" (since the available number is actually computed in a thread-safe way), maybe clarify that the returned number of available tokens does not guarantee calls like Take/TakeAvailable will succeed (since the available tokens could have changed in the meantime), and that this is primarily for introspection/metrics/reporting/debugging?
There was a problem hiding this comment.
That's a good idea, thanks.
@hongchaodeng How about this?
// Available returns the number of available tokens.
// It will be negative when there are consumers waiting for tokens.
// Note that if this returns greater than zero,
// it does not guarantee that calls that take tokens
// from the buffer will succeed, as the number of available
// tokens could have changed in the meantime.
// This method is intended primarily for metrics reporting
// and debugging.
There was a problem hiding this comment.
Actually, I'll land this now and make that change in another PR.
Bucket: expose Available() and Capacity()
Hi,
The current token bucket implementation is great. Nonetheless we need some underlying data to implement more powerful functionalities.
For example, if we want to implement a function
CanAcceptwhich only checks available tokens without taking it; implement a function that exports rate limiter's metrics to better understand current flow and bottleneck on our services. Both functions will benefit if we can get the underlying data likeavailandcapacity, including the already publicRate().Let me know if there are questions. Highly appreciate it if anyone can help review this issue. Thanks!