feat: add backoff and jitter to retry #643
Conversation
retry.go
Outdated
| defer timer.Stop() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
We should not wait for the context deadline but check whether the deadline will be exceeded or not:
dl, ok := ctx.Deadline()
if !ok || time.Until(dl) > delay {
time.Sleep(delay)
}There was a problem hiding this comment.
Is there no need to respect context cancel ?
There was a problem hiding this comment.
It is better to respect the cancellation, but the returned errAbortWaiting complicates the code outside.
How about we do this, removing the errAbortWaiting:
ctx := context.Background()
if dl, ok := ctx.Deadline(); !ok || time.Until(dl) > delay {
if ch := ctx.Done(); ch != nil {
tm := time.NewTimer(delay)
defer tm.Stop()
select {
case <-ch:
case <-tm.C:
}
} else {
time.Sleep(delay)
}
return true
}
return falseThere was a problem hiding this comment.
We still let it retry if the context is ended. The underlying pipe will check the ctx.Err().
There was a problem hiding this comment.
outside code of
WaitUntilNextRetrycan be complicated.
I feel that this is bit more code style. when not returning error, caller should always check context is done or ,as you can said, pipe always will check and all contributors should know that. It is unclear to me, so that complicated but explicit code is more sense to me. I change it, but can you give me more detailed reason?
There was a problem hiding this comment.
pipe always will check and all contributors should know that. It is unclear to me
It is indeed implicit. But the reason for not returning an error is simple: adding a new error path to replace a goto retry statement seems doing too much.
retry.go
Outdated
| base := util.FastRandFloat64() | ||
| backoff := uint64(1 << uint64(math.Min(float64(attempts), maxAttemptsShift))) | ||
| jitter := base * float64(backoff) * float64(time.Millisecond) | ||
| return time.Duration(math.Min(jitter, float64(defaultMaxRetryDelay))) |
There was a problem hiding this comment.
We should apply randomness only to the jitter part. The delay produced by the current implementation is not monotonically increasing.
Can we avoid using float64? We can use the built-in min and max functions.
There was a problem hiding this comment.
For example:
backoff := time.Microsecond * 5 * (1 << min(20, attempts))
jitter := time.Microsecond * time.Duration(rand.Int64N(1<<min(20, attempts)))
return min(time.Second, backoff+jitter)There was a problem hiding this comment.
Well, actually above logic came from here.
The delay produced by the current implementation is not monotonically increasing.
Intended part. To smooth multiple client calls.
| if err != nil { | ||
| return newErrResult(err) | ||
| } |
| if err != nil { | ||
| return fillErrs(len(multi), err) | ||
| } |
| if err != nil { | ||
| return newErrResult(err) | ||
| } |
|
|
||
| if !isWaitingForRetry { | ||
| shouldRetry := c.retryHandler.WaitUntilNextRetry( | ||
| ctx, attempts, resp.Error(), | ||
| ) | ||
|
|
||
| if !shouldRetry { | ||
| continue | ||
| } | ||
|
|
||
| isWaitingForRetry = true | ||
| } |
There was a problem hiding this comment.
Why do you put the waiting here? Instead of putting it at:
Lines 679 to 681 in fc348f5
close: #633
Adding retry policy.
default is exponential backoff and jitter.
clean logic for retry handler is not easy. If have a idea, give me review.