Add timeout to token refresh retry #123
Conversation
src/types.rs
Outdated
| @@ -66,6 +66,8 @@ impl HttpClient { | |||
| retries += 1; | |||
| if retries >= RETRY_COUNT { | |||
| return Err(err); | |||
| } else { | |||
There was a problem hiding this comment.
No else necessary here, since the if block returns.
Please import sleep() and Duration (from std, not via Tokio).
How long does a loop iteration take for you without timeout? I wonder if 200ms is a good length or if we could go a bit shorter on the first retry and back off a little?
There was a problem hiding this comment.
sleep from std?
I'm not sure if that's good idea in async code.
So in my case we had metadata server rejecting connections due sudden burst in which case multiple retries in short interval would be only even more harmful
I used 200ms just to make sure retries are done within 1 second
There was a problem hiding this comment.
How long does a loop iteration take for you without timeout?
Current logic (no timeout) performs retries within 10 millisecond
I do not think it is good idea for metadata server because google most likely has some burst limits as we started encountering it recently with GKE's scheduled pods
There was a problem hiding this comment.
sleep from std? I'm not sure if that's good idea in async code.
No, Duration from std, but would like to import sleep() so that you can use it without the tokio::time prefix at the callsite.
So in my case we had metadata server rejecting connections due sudden burst in which case multiple retries in short interval would be only even more harmful I used 200ms just to make sure retries are done within 1 second
Okay, how about we started with like 50ms and multiply by 2 on each iteration?
- 50ms
- 100ms
- 200ms
- 400ms
- 800ms
That gets the total time to 1.5s.
|
(If you rebase, the MSRV failure in CI should go away.) |
DoumanAsh
force-pushed
the
fix_bad_retry
branch
from
1d90c3a to
966919e
Compare
|
Rebase PR and applied comments |
Fixes #122