-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
Fix Retry-After in clients #26874
Fix Retry-After in clients #26874
Conversation
Looks good to me :) |
What's the point of the backoff mgr now? |
@smarterclayton - it's used in bunch of other places (e.g. line 807). We may consider a different fix, but currently "Retry-After" simply doesn't work at all - see #26871 |
So maybe we shouldn't be using NoBackoff, we should be using a new On Mon, Jun 6, 2016 at 10:55 AM, Wojciech Tyczynski <
|
Maybe the backoff interface doesn't need However, in the test, I would like a way to stub out sleep with a function that can record the number of times it was called and with what arguments for testing. I don't want to introduce actual sleeps into the tests. |
@krousey - your proposal sounds reasonable for me - I will fix the PR tomorrow. |
@krousey @smarterclayton - I changed it (not really as you suggested, but hopefully it's good enough). PTAL |
Change LGTM (in terms of what we discussed) |
if len(b.expectedSleeps) != 0 { | ||
b.t.Errorf("some sleeps weren't called: %v", b.expectedSleeps) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testBackoffManager
should probably just record the sleep calls and leave the verification to the caller. This would allow for more test specific verification, put the error messages in the test cases, and de-tangle this struct from a *testing.T
member.
@krousey - comments applied. PTAL |
|
||
func (b *testBackoffManager) Sleep(d time.Duration) { | ||
if b.sleeps == nil { | ||
b.sleeps = make([]time.Duration, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can append to a nil slice. No need to explicitly allocate.
One minor nit, but LGTM otherwise. |
@krousey - thanks a lot for review - the last comment applied |
GCE e2e build/test passed for commit 528713b. |
Automatic merge from submit-queue |
Fix #26871