Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/time/rate: convert Limit to an Interface #43003

Open
deefdragon opened this issue Dec 4, 2020 · 3 comments
Open

x/time/rate: convert Limit to an Interface #43003

deefdragon opened this issue Dec 4, 2020 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@deefdragon
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.15.5 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOOS="linux"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build868185860=/tmp/go-build -gno-record-gcc-switches"

What did you do?

examined using rate.Limiter to interact with an API

What did you expect to see?

A way to interact with an API where the api implements token bucket rate limiting, or The ability to create an implementation of the rate limit interface to do so.

What did you see instead?

rate.Limiter is not an interface.

Limiter should likely be an interface providing the programmer the options to choose between a token bucket or a leaky bucket implementations, if not sliding and fixed windows as well.

Looking at the API, Allow, Wait and Reserve (and their N counterparts) would all be part of the interface, while get/set limit and get/set burst would likely be specific to only some implementations, and so not part of the interface.
@gopherbot gopherbot added this to the Unreleased milestone Dec 4, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 4, 2020
@cagedmantis cagedmantis changed the title x/time/rate: Convert Limit to an Interface x/time/rate: convert Limit to an Interface Dec 4, 2020
deefdragon pushed a commit to deefdragon/go-x-time that referenced this issue Dec 17, 2020
x/time/rate: convert Limiter and Reservation into interfaces
f445350 
Conversion into interfaces allows for easier implementation and swapping of rate limit algorithms such as leaky bucket, fixed window, or sliding window. The original Limiter struct as been converted into the TokenBucket struct. The original Reservation struct has been converted into the TokenBucketReservation struct. The API remains backwards compatable, with NewLimiter() returning a TokenBucket via a call to NewTokenLimiter().

fixes golang/go#43003
deefdragon added a commit to deefdragon/go-x-time that referenced this issue Dec 17, 2020
@deefdragon
x/time/rate: convert Limiter and Reservation into interfaces
6e88d5f 
Conversion into interfaces allows for easier implementation and swapping of rate limit algorithms such as leaky bucket, fixed window, or sliding window. The original Limiter struct as been converted into the TokenBucket struct. The original Reservation struct has been converted into the TokenBucketReservation struct. The API remains backwards compatable, with NewLimiter() returning a TokenBucket via a call to NewTokenLimiter().

fixes golang/go#43003
@deefdragon
Copy link
Author

I just threw together a quick migration to an interface to play with here. I had to make the reservation returned by Reserve() an interface as well when thinking forward about implementation of other rate limit algorithms.

I think the name of the Reservation interface should be kept to Reservation, breaking the 'er' rule, but it just works better than anything else I could find.

The only other changes to the API would be the addition of NewTokenBucket(). NewLimiter() then just calls NewTokenBucket(), which should preserve all existing functionality.

I wouldn't have jumped the gun on the proposal process, but I need a sliding window implementation soon, and if i'm writing it anyway, I might as well do so in a manner than benefits everyone.

@cespare
Copy link
Contributor

cespare commented Dec 17, 2020

The x/time/rate package only has one implementation, so why does it need an interface? If you need an abstraction across multiple types of rate limiters, that can live in your code.

To quote the Code Review Comments page:

Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values. The implementing package should return concrete (usually pointer or struct) types: that way, new methods can be added to implementations without requiring extensive refactoring.

@cespare
Copy link
Contributor

cespare commented Dec 17, 2020

(In any case, it can't be changed now in a backward-compatible way.)

@deefdragon deefdragon mentioned this issue Feb 17, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@cagedmantis @cespare @deefdragon @gopherbot