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
Comments
@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.) |
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. |
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. |
I'd like to tackle this if it's still something that needs to be done. |
@simar7 - Do you have a real-world use case where you need to update the burst value ? |
Change https://golang.org/cl/184082 mentions this issue: |
Does this CR need a bump? |
I have done that now. |
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 ***@***.***> wrote:
I have done that now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23575?email_source=notifications&email_token=ACKIVXMSX2G6IRQNQMBLA4LQKC27FA5CNFSM4EN3UBL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD64FCEY#issuecomment-532173075>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACKIVXI5KDX6P2SUCDSKQK3QKC27FANCNFSM4EN3UBLQ>
.
|
Thanks for the feedback. I've addressed the comment in the review. |
Awesome, thank you!! |
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.
The text was updated successfully, but these errors were encountered: