add onRetryCallback callback function #405
Conversation
interceptors/retry/options.go
Outdated
| // OnRetryCallback called in retry attempts flow, which can be used to add additiional logic when retry occurs | ||
| // | ||
| type OnRetryCallback func(attempt uint, err error) |
There was a problem hiding this comment.
No need for both this and OnRetryCallbackContext, just merge both into OnRetryCallback with a context.Context parameter first.
interceptors/retry/options.go
Outdated
| // WithOnRetryCallback sets the `OnRetryCallback` used to add additional logic when retry occurs. | ||
| func WithOnRetryCallback(fn OnRetryCallback) CallOption { | ||
| return CallOption{applyFunc: func(o *options) { | ||
| o.onRetryCallback = OnRetryCallbackContext(func(ctx context.Context, attempt uint, err error) { | ||
| fn(attempt, err) | ||
| }) | ||
| }} | ||
| } | ||
|
|
||
| // WithOnRetryCallbackContext sets the `OnRetryCallbackContext` used to add additional logic when retry occurs. | ||
| func WithOnRetryCallbackContext(fn OnRetryCallbackContext) CallOption { | ||
| return CallOption{applyFunc: func(o *options) { | ||
| o.onRetryCallback = fn | ||
| }} | ||
| } |
There was a problem hiding this comment.
Re: merge these into one
interceptors/retry/retry.go
Outdated
| @@ -48,6 +48,7 @@ func UnaryClientInterceptor(optFuncs ...CallOption) grpc.UnaryClientInterceptor | |||
| if lastErr == nil { | |||
| return nil | |||
| } | |||
| callOpts.onRetryCallback(callCtx, attempt, lastErr) | |||
| logTrace(parentCtx, "grpc_retry attempt: %d, got err: %v", attempt, lastErr) | |||
There was a problem hiding this comment.
Maybe we can make this log here the default retry callback behaviour while we're here?
There was a problem hiding this comment.
I can do it, but If we do this, we will lose this trace log once the custom callback will be set by the user.
There was a problem hiding this comment.
We'll document that as the default behaviour so that the user knows that it will disappear if they override the callback. They can always manually add it back again.
There was a problem hiding this comment.
I can also put this in the WithOnRetryCallback before invoking the user's logic, if this makes sense.
There was a problem hiding this comment.
No I think one of the things I want to do here is allow the user to disable this log behaviour too.
interceptors/retry/options.go
Outdated
| // OnRetryCallback called in retry attempts flow, which can be used to add additiional logic when retry occurs | ||
| // | ||
| // The context can be used to extract request scoped metadata and context values. |
There was a problem hiding this comment.
After merging these, lets simplify this comment a bit too.
| // OnRetryCallback called in retry attempts flow, which can be used to add additiional logic when retry occurs | |
| // | |
| // The context can be used to extract request scoped metadata and context values. | |
| // OnRetryCallback is the type of function called when a retry occurs. |
interceptors/retry/options.go
Outdated
| @@ -75,6 +87,22 @@ func WithBackoffContext(bf BackoffFuncContext) CallOption { | |||
| }} | |||
| } | |||
|
|
|||
| // WithOnRetryCallback sets the `OnRetryCallback` used to add additional logic when retry occurs. | |||
There was a problem hiding this comment.
| // WithOnRetryCallback sets the `OnRetryCallback` used to add additional logic when retry occurs. | |
| // WithOnRetryCallback sets the callback to use when a retry occurs. | |
| // | |
| // By default, no action is performed on retry. |
Codecov Report
@@ Coverage Diff @@
## v2 #405 +/- ##
===========================================
- Coverage 83.58% 56.14% -27.44%
===========================================
Files 30 38 +8
Lines 932 1334 +402
===========================================
- Hits 779 749 -30
- Misses 114 514 +400
- Partials 39 71 +32
Continue to review full report at Codecov.
|
|
Thanks for review @johanbrandhorst |
|
Thanks for your contribution! |
Implements the suggested solutoin in #356