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.

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

x/time/rate: SetLimit does not update burst #23575

Closed
madhuravi opened this issue Jan 26, 2018 · 13 comments
Closed

x/time/rate: SetLimit does not update burst #23575

madhuravi opened this issue Jan 26, 2018 · 13 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@madhuravi
Copy link

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.

@bradfitz bradfitz changed the title "x/time:" SetLimit does not update burst x/time/rate: SetLimit does not update burst Jan 27, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jan 27, 2018
@meirf
Copy link
Contributor

meirf commented Jun 16, 2018

@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.)

@agnivade agnivade added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jun 17, 2018
@Sajmani
Copy link
Contributor

Sajmani commented Jun 29, 2018

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
Copy link
Author

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.

@agnivade agnivade added the Suggested Issues that may be good for new contributors looking for work to do. label Jun 2, 2019
@simar7
Copy link

simar7 commented Jun 18, 2019

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

@agnivade
Copy link
Contributor

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

@mholt
Copy link

mholt commented Jun 21, 2019

@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
Copy link
Contributor

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

@gopherbot
Copy link

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

@mholt
Copy link

mholt commented Sep 17, 2019

Does this CR need a bump?

@agnivade
Copy link
Contributor

I have done that now.

@Sajmani
Copy link
Contributor

Sajmani commented Sep 18, 2019 via email

@simar7
Copy link

simar7 commented Sep 19, 2019

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

@mholt
Copy link

mholt commented Sep 21, 2019

Awesome, thank you!!

@golang golang locked and limited conversation to collaborators Sep 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

7 participants