More generic API to work with x/time/rate #582
Conversation
|
@nelz9999 this looks awesome to me and should fulfill our license limitations while also fulfilling Peter's API request. Thank you! |
|
Very nice man. |
ratelimit/token_bucket_test.go
Outdated
| "testing" | ||
| "time" | ||
|
|
||
| jujuratelimit "github.com/juju/ratelimit" | ||
|
|
||
| "github.com/go-kit/kit/endpoint" | ||
| "github.com/go-kit/kit/ratelimit" | ||
| "golang.org/x/time/rate" |
There was a problem hiding this comment.
Can you move this import decl up with juju, above?
ratelimit/token_bucket.go
Outdated
| // exceed the maximum request rate are delayed via the parameterized sleep | ||
| // function. By default you may pass time.Sleep. | ||
| func NewTokenBucketThrottler(tb *ratelimit.Bucket, sleep func(time.Duration)) endpoint.Middleware { | ||
| // return NewDelayingLimiter(NewWaiter(tb)) |
There was a problem hiding this comment.
Is there a reason this didn't work? Would be cool if we could express all the old functions in terms of the new ones!
There was a problem hiding this comment.
I can do it, but then the sleep parameter gets ignored. I'll check in that change, so you can evaluate if you like it.
|
@peterbourgon This is ready for you to see if you like what I've done to marry the old functions to the new ones. |
|
Yep, I'm happy with this for the current point release. For the next one I think we can totally drop Juju. Thanks so much for the work! 👍 |
|
Urgh, build failures are unrelated: see openzipkin-contrib/zipkin-go-opentracing#68 |
|
Build failures due to Thrift have been fixed in zipkin-go-opentracing Go kit also has issue with thrift update, PR #585 resolves this. |
|
@peterbourgon Now that 0.6.0 has been cut, would you like the follow-on PR to remove |
|
@nelz9999 Sure. |
As per the discussion in Slack (https://gophers.slack.com/archives/C04S3T99A/p1500582764076547), I added the ability to use the
x/time/ratepackage for the rate limiting implementation, while also keeping backwards-compatible with thejuju/ratelimitusage.I'm pretty pleased with the structure, using subset interfaces off the
x/time/rate.Limiterto express most of what we needed.I was even able to port the older usage of
NewTokenBucketLimiterto use the new implementation under the covers, but was unable to do so withNewTokenBucketThrottler(because IMO the API leaked testing configuration).For any future API-breaking versions, it wouldn't be too hard to make the
juju-related adapters a sub-package, but I'm not pushing that in this PR.Would love to hear solid opinions on improvements I could make to the names.