Retry callbacks passing errors.#370
Retry callbacks passing errors.#370peterbourgon merged 11 commits intogo-kit:masterfrom rossmcf:retry_cb
Conversation
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
Thanks very much for the submission! There's still some cleanup and reorg to be done before this will pass the bar, but I'm pretty confident we can get it there :)
| // Should return whether the Retry function should continue trying, and a custom | ||
| // error message if desired. The error message may be nil, but a true/false | ||
| // is always expected. In all cases if the error message is supplied, the | ||
| // current error will be replaced. |
There was a problem hiding this comment.
Some strange extra spaces at the beginning of the comment lines here? Also, please wrap at 80 col.
There was a problem hiding this comment.
// Callback is a function that...There was a problem hiding this comment.
OK. I think this is sorted now. Still getting to grips with vim…
| // error message if desired. The error message may be nil, but a true/false | ||
| // is always expected. In all cases if the error message is supplied, the | ||
| // current error will be replaced. | ||
| type Callback func(n int, err error) (cont bool, cbErr error) |
There was a problem hiding this comment.
Maybe...
type Callback func(n int, received error) (keepTrying bool, err error)There was a problem hiding this comment.
Updated to make the intention clearer.
| // up to max times, or until the timeout is elapsed, whichever comes first. | ||
| func Retry(max int, timeout time.Duration, b Balancer) endpoint.Endpoint { | ||
| return RetryWithCallback(max, timeout, b, func(c int, err error) (bool, error) { return true, nil }) | ||
| } |
There was a problem hiding this comment.
So I haven't given this enough thought to give you really actionable advice yet, but I'm not happy with the way this is currently structured. Namely I'd want the "retry up to max times, or until timeout, whichever comes first" logic to be encoded in the callback passed to the function, and not hard-coded into the function itself. I'm not sure how it would look, precisely. Can you see if that's possible?
There was a problem hiding this comment.
I've split the max retry logic out into the callback. There's a helper function for generating a max retries callback if that's all you want to do, and Retry() passes one of those on to RetryWithCallback() now.
I'm not sure if or how to tackle handling timeouts in the callback. I think it's reasonable to keep those two jobs separate. To me, I'm happy with the timeout in the select inside RetryWithCallback(). What do you reckon @peterbourgon?
| func RetryWithCallback(max int, timeout time.Duration, b Balancer, cb Callback) endpoint.Endpoint { | ||
| if cb == nil { | ||
| panic("nil Callback") | ||
| } |
There was a problem hiding this comment.
We can be safer, here. If there's a nil callback, we can substitute a no-op or always-pass implementation instead.
There was a problem hiding this comment.
Substituting a no-op now, with a test.
| if cbErr != nil { | ||
| currentErr = cbErr.Error() | ||
| } | ||
| a = append(a, currentErr) |
There was a problem hiding this comment.
This is super messy, especially regarding the final error returned to the caller, which at this point I think deserves to be lifted into its own type, with a slice of underlying errors as a member. But, I think a lot of the messiness may go away if you're able to implement all of this logic in the callback as I wanted above.
- RetryError captures more sophisticated error behaviors - Simplify the error branch in the switch significantly - Clean up the tests a little bit - Comment cleanup and rewrap
sd/lb: updates to Retry changes
|
Ace! |
Retry callbacks passing errors.
2 participants
Hopefully this should resolve #341.
I wasn't sure what the protocol should be for this, but I noticed things had gone a bit quiet on the other PR for this issue. I've pulled @morganhein's changes and made a few tweaks in line with @peterbourgon's feedback.
Callbacks now accept an
error, and an additional test is added to demonstrate that errors are passed unchanged from the endpoint.Please let me know if anything needs tweaked, or if I should submit this change differently.