Introduce retry looping. #5
Conversation
There was a problem hiding this comment.
Having loop.Error() solves one of my big concerns with rogpeppe's implemention, as exiting the loop early is clearly flagged as an Error condition (DurationExceeded, etc).
Which means that you know with a
if loop.Error() != nil {
}
outside of the loop whether it exited because it ran out of retries, or it exited because it was successful.
Passing "err" into the Next() function feels weird, but it is the piece that enables this. AFAICT
| // Delay specifies how long to wait between retries. | ||
| Delay time.Duration | ||
|
|
||
| // MaxDelay specifies how longest time to wait between retries. If no |
| // `retry.DoubleDelay` can be used that will provide an exponential | ||
| // backoff. The first time this function is called attempt is 1, the | ||
| // second time, attempt is 2 and so on. | ||
| BackoffFunc func(delay time.Duration, attempt int) time.Duration |
There was a problem hiding this comment.
Can you also comment about what 'delay' is being passed in? From the "BackoffFactor" function, it looks to be the time of the last delay. Else you would do delay * pow(time.Duration(factor), attempt)
But that doesn't seem to be clearly documented here.
I could go with either "base delay" or "last delay" but we should be clear which is used.
| // BackoffFactor returns a new LoopSpec with the backoff function set to | ||
| // scale the backoff each time by the factor specified. This is an example | ||
| // of the syntactic sugar that could be applied to the spec structures. | ||
| func (spec LoopSpec) BackoffFactor(factor int) LoopSpec { |
There was a problem hiding this comment.
I would have expected factor to actually be a float, as then you can do things like "50% longer each cycle" (factor =1.5)
| return errors.NotValidf("missing Clock") | ||
| } | ||
| // One of Attempts or MaxDuration need to be specified | ||
| if args.Attempts == 0 && args.MaxDuration == 0 && args.Stop == nil { |
There was a problem hiding this comment.
Given your earlier wording, isn't Attempts <= 0 considered disabled?
| close(stop) | ||
|
|
||
| s.spec.Stop = stop | ||
| var err error |
There was a problem hiding this comment.
you should initialize 'err' to something other than 'nil' else 'err = success()' isn't really evaluated. I suppose it is covered by called = true, but it seems odd. You could actually avoid the extra variable with just
err := &ErrNotCalled{}
or something along those lines.
| start := args.Clock.Now() | ||
| for i := 1; args.Attempts <= 0 || i <= args.Attempts; i++ { | ||
|
|
||
| spec := LoopSpec{ |
There was a problem hiding this comment.
I'm very happy to see one form implemented with the other, as then you help ensure they evolve together.
|
My 2 cents as somebody who just recently started using juju/retry in snapd: The main issue is there are cases where e.g. a HTTP request doesn't technically fail (err is nil), but I still want to retry based on http response code, e.g. on 500 or 503. To handle this case in existing implementation or the one from this PR I'm forced to introduce my own error type just to satisfy the needs of retry loop handling. The other "oddity" is the LastError method which - as is - packs the error I'm most interested into a "unexpected error type: ..." string, removing the ability to inspect actual error type; to avoid this I need to implement NotifyFunc handler just to keep track of my error. Perhaps this PR and rogpeppe's could be combined? |
There was a problem hiding this comment.
I think this is quite good but I do wonder (as per other reviewers) whether always tying retries to errors is really the best approach. Certainly this will be a common use case (and there should be helpers to do that easily) but it perhaps shouldn't be the only way.
It might be good to compare to Roger's work and see what can be incorporated from that.
| c.Assert(s.clock.delays, gc.HasLen, 3) | ||
| } | ||
|
|
||
| func (s *loopSuite) TestMulitipleLoops(c *gc.C) { |
| @@ -163,8 +163,8 @@ func (args *CallArgs) Validate() error { | |||
| return errors.NotValidf("missing Clock") | |||
| } | |||
| // One of Attempts or MaxDuration need to be specified | |||
| if args.Attempts == 0 && args.MaxDuration == 0 { | |||
| return errors.NotValidf("missing Attempts or MaxDuration") | |||
| if args.Attempts == 0 && args.MaxDuration == 0 && args.Stop == nil { | |||
There was a problem hiding this comment.
This is nice. It's important that at least one way of stopping the retries is in place.
|
|
||
| func (s *loopSuite) TestCalledOnceEvenIfStopped(c *gc.C) { | ||
| stop := make(chan struct{}) | ||
| called := false |
| c.Assert(s.clock.delays, gc.HasLen, 0) | ||
| } | ||
|
|
||
| func (s *loopSuite) TestCalledOnceEvenIfStopped(c *gc.C) { |
There was a problem hiding this comment.
do you actually want this behaviour? I would have thought that if the stop channel is closed it's game over, you don't want the loop at all.
After listening to users of the retry Call method, it became clear that it isn't always useful to just have a single func to call.
This introduces retry.Loop which provides a mechanism to use inside for loops more naturally.