search

juju / ratelimit

Efficient token-bucket-based rate limiter package.
Other
2.79k stars 312 forks source link

Bucket: expose Available() and Capacity() #10

Closed hongchaodeng closed 8 years ago

hongchaodeng commented 8 years ago

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 CanAccept which 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 like avail and capacity, including the already public Rate().

Let me know if there are questions. Highly appreciate it if anyone can help review this issue. Thanks!

axw commented 8 years ago

Seems fine to me, with one small fix; deferring to @rogpeppe, the author, though.

rogpeppe commented 8 years ago

Thanks for this. This is good in principle, but could do with a little work to make it correct and tested.

Please rebase to a single commit before re-proposing, thanks.

hongchaodeng commented 8 years ago

Thanks @rogpeppe for your reply. I realized my mistake and fixed it. I have also added a test for the internal available().

I squash all I want to do in single commit. However, I add another commit to update gocheck and math.Abs -- the second one made the test not working.

Let me know if there are questions. Thanks again!

hongchaodeng commented 8 years ago

@rogpeppe Changed naming to TestAvailable()

hongchaodeng commented 8 years ago

@rogpeppe Addressed all comments. PTAL.

rogpeppe commented 8 years ago

Thanks very much for making these changes. LGTM modulo some comments above.

rogpeppe commented 8 years ago

LGTM with one trivial suggestion.