Go: x/time/rate: SetLimit does not update burst

[madhuravi]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.9.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/madhuravi/Uber/gocode"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.9.1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.9.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0k/vpbbbbjx3vv14fjd8l9ry2fh0000gn/T/go-build455608000=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Documentation for SetLimiter https://github.com/golang/time/blob/master/rate/rate.go#L276
"// SetLimitAt sets a new Limit for the limiter. The new Limit, and Burst, may be violated
// or underutilized by those which reserved (using Reserve or Wait) but did not yet act
// before SetLimitAt was called."

What did you expect to see?

I expect SetLimit to update burst value based on above comment. But it does not take an argument for or update the burst value at all.

What did you see instead?

I would like a function to update the burst value. I manage this on my own with locks etc. but would be nice to be part of the library.

[meirf]

@Sajmani any technical reason setting burst was left out?

@madhuravi any interest in contributing? Even if setting burst was intentionally omitted, the documentation could probably be clearer to avoid the confusion you encountered. The guide was recently overhauled/streamlined. (Though we are currently in a freeze, x/time/rate is not bound by that freeze.)

[Sajmani]

SetLimit sets the limit only, not the burst. The comment is explaining that this change may violate the limit and burst settings of outstanding reservations.

There is no SetBurst because to date no one has asked for it. If you have a use case, I think we can accept a contribution for it. I'll need to pull in a reviewer, as I'm no longer actively working with this code.

[madhuravi]

This was for my previous project, I'm no longer working on it. IMO it's worth clarifying the comment but feel free to close if there are no other requests for this.

[simar7]

I'd like to tackle this if it's still something that needs to be done.

[agnivade]

@simar7 - Do you have a real-world use case where you need to update the burst value ?

[mholt]

@agnivade I do. My use case is I have a long-lived rate limiter shared across goroutines that has to adapt to dynamic configuration changes, including burst size.

@simar7 I would love it if you contributed something! Tag me and I'll help review if that's allowed.

[agnivade]

Sounds reasonable @mholt. @simar7 - feel free to have a go at this.

[gopherbot]

Change https://golang.org/cl/184082 mentions this issue: rate: Add SetBurst() to dynamically update burst size

[mholt]

Does this CR need a bump?

[agnivade]

I have done that now.

[Sajmani]

Apologies, I've been away from this email. I've replied on the review.

On Tue, Sep 17, 2019 at 7:07 AM Agniva De Sarker [e-mail]
wrote:

I have done that now.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/23575?email_source=notifications&email_token=ACKIVXMSX2G6IRQNQMBLA4LQKC27FA5CNFSM4EN3UBL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD64FCEY#issuecomment-532173075,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACKIVXI5KDX6P2SUCDSKQK3QKC27FANCNFSM4EN3UBLQ
.

[simar7]

Thanks for the feedback. I've addressed the comment in the review.

[mholt]

Awesome, thank you!!