s3: introduce retrying on known errors

s3: introduce retrying on known errors



https://code.launchpad.net/~niemeyer/goamz/introduce-retrying/+merge/146042

(do not edit description out of merge proposal)

[niemeyer, 2013-02-01 05:56:24]

Please take a look.

[rog, 2013-02-01 10:04:01]

very nice to see this, thanks!

a few comments below, though nothing major.

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go
File aws/attempt.go (right):

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go#newcode29
aws/attempt.go:29: func (a *Attempt) Next() bool {
this implementation seems like it could be simpler now that we've got force.

how about something like this? (interface and semantics unchanged)

// AttemptStrategy represents a strategy for waiting for an action
// to complete successfully. This is an internal type used by the
// implementation of other goamz packages.
type AttemptStrategy struct {
	Total time.Duration // total duration of attempt.
	Delay time.Duration // interval between each try in the burst.
}

type Attempt struct {
	strategy AttemptStrategy
	last     time.Time
	end      time.Time
	force    bool
}

// Start begins a new sequence of attempts for the given strategy.
func (s AttemptStrategy) Start() *Attempt {
	now := time.Now()
	return &Attempt{
		strategy: s,
		last: now.Add(-s.Delay),		// first attempt will happen immediately.
		end: now.Add(s.Total),
		force: true,				// always force at least one attempt.
	}
}

// Next waits until it is time to perform the next attempt or returns
// false if it is time to stop trying.
func (a *Attempt) Next() bool {
	now := time.Now()
	if !a.force && !now.Before(a.end) {
		return false
	}
	a.force = false
	time.Sleep(a.strategy.Delay - now.Sub(a.last))
	a.last = time.Now()
	return true
}

// HasNext returns whether another attempt will be made if the current
// one fails. If it returns true, the following call to Next is
// guaranteed to return true.
func (a *Attempt) HasNext() bool {
	if a.force || time.Now().Before(a.end) {
		a.force = true
		return true
	}
	return false
}

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go#newcode42
aws/attempt.go:42: if sleep := a.strategy.Delay - now.Sub(a.last); sleep > 0 {
i'm not entirely sure that it's more useful to have a determined interval
between attempt starts rather than between an attempt ending and the next one
starting.

assuming that's desired though, there's no need for a loop
here. time.Sleep already includes a similar loop.

    time.Sleep(a.strategy.Delay - now.Sub(a.last))

should work fine.

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go#newcode53
aws/attempt.go:53: func (a *Attempt) HasNext() bool {
"HasNext" reads a little awkwardly here. "next" isn't really something that can
be "had".

how about "More"?
(NextOk or WillNext are also possibilities)

alternatively you could switch the sense and have "Done" or "IsDone" (AtEnd?
Complete(d), Finished?)

https://codereview.appspot.com/7264043/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/7264043/diff/1/s3/s3.go#newcode159
s3/s3.go:159: if shouldRetry(err) && attempt.HasNext() {
nice idiom.

https://codereview.appspot.com/7264043/diff/1/s3/s3.go#newcode311
s3/s3.go:311: println("Retrying List.")
d

https://codereview.appspot.com/7264043/diff/1/s3/s3.go#newcode528
s3/s3.go:528: switch e := err.(type) {
io.ErrUnexpectedEOF is one i've seen in the past, although i haven't seen it for
a while.

https://codereview.appspot.com/7264043/diff/1/s3/s3.go#newcode531
s3/s3.go:531: case *net.OpError:
this seems very broad. we don't want to retry for 5 seconds if we've got an
address parsing error, for example.
i'd be tempted to restrict it to Op=="remote error" and see how it goes.

https://codereview.appspot.com/7264043/diff/1/s3/s3_test.go
File s3/s3_test.go (right):

https://codereview.appspot.com/7264043/diff/1/s3/s3_test.go#newcode32
s3/s3_test.go:32: s3.SetAttemptStrategy(&aws.AttemptStrategy{Total: 500 *
time.Millisecond, Delay: 100 * time.Millisecond})
i'd be tempted to split this line.

https://codereview.appspot.com/7264043/diff/1/s3/s3i_test.go
File s3/s3i_test.go (right):

https://codereview.appspot.com/7264043/diff/1/s3/s3i_test.go#newcode108
s3/s3i_test.go:108: for attempt := attempts.Start(); attempt.Next(); {
given that DelBucket retries, can't you lose this loop now and its associated
error tests?

[niemeyer, 2013-02-01 18:55:34]

Please take a look.

[niemeyer, 2013-02-01 20:21:58]

Please take a look.

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go
File aws/attempt.go (right):

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go#newcode29
aws/attempt.go:29: func (a *Attempt) Next() bool {
On 2013/02/01 10:04:01, rog wrote:
> this implementation seems like it could be simpler now that we've got force.

I've changed the implementation to support Min.

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go#newcode42
aws/attempt.go:42: if sleep := a.strategy.Delay - now.Sub(a.last); sleep > 0 {
On 2013/02/01 10:04:01, rog wrote:
> i'm not entirely sure that it's more useful to have a 
determined interval
> between attempt starts rather than between an attempt ending and the next one

It actually makes a significant difference. We should import this back into
juju-core later. If a request takes 2 seconds to respond, there's no reason for
waiting any further.

https://codereview.appspot.com/7264043/diff/1/aws/attempt.go#newcode53
aws/attempt.go:53: func (a *Attempt) HasNext() bool {
On 2013/02/01 10:04:01, rog wrote:
> "HasNext" reads a little awkwardly here. "next" isn't really something that
can
> be "had".

Funny enough, it was More originally, but that's confusing because More and Next
are too similar in the context, and error prone because their result is of the
same type.

It's also grammatically okay. "Joe has the next attempt to succeed."

> alternatively you could switch the sense and have "Done" or "IsDone" (AtEnd?
> Complete(d), Finished?)

None of these are nearly as clear in usage as Next and HasNext.

https://codereview.appspot.com/7264043/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/7264043/diff/1/s3/s3.go#newcode311
s3/s3.go:311: println("Retrying List.")
On 2013/02/01 10:04:01, rog wrote:
> d

Done.

https://codereview.appspot.com/7264043/diff/1/s3/s3.go#newcode528
s3/s3.go:528: switch e := err.(type) {
On 2013/02/01 10:04:01, rog wrote:
> io.ErrUnexpectedEOF is one i've seen in the past, although i haven't seen it
for
> a while.

Done.

https://codereview.appspot.com/7264043/diff/1/s3/s3.go#newcode531
s3/s3.go:531: case *net.OpError:
On 2013/02/01 10:04:01, rog wrote:
> this seems very broad. we don't want to retry for 5 seconds if we've got an
> address parsing error, for example.
> i'd be tempted to restrict it to Op=="remote error" and see how it goes.

True. I'll restrict to "read" and "write".

https://codereview.appspot.com/7264043/diff/1/s3/s3_test.go
File s3/s3_test.go (right):

https://codereview.appspot.com/7264043/diff/1/s3/s3_test.go#newcode32
s3/s3_test.go:32: s3.SetAttemptStrategy(&aws.AttemptStrategy{Total: 500 *
time.Millisecond, Delay: 100 * time.Millisecond})
On 2013/02/01 10:04:01, rog wrote:
> i'd be tempted to split this line.

Done.

https://codereview.appspot.com/7264043/diff/1/s3/s3i_test.go
File s3/s3i_test.go (right):

https://codereview.appspot.com/7264043/diff/1/s3/s3i_test.go#newcode108
s3/s3i_test.go:108: for attempt := attempts.Start(); attempt.Next(); {
On 2013/02/01 10:04:01, rog wrote:
> given that DelBucket retries, can't you lose this loop now and its associated
> error tests?

No, all that logic is relevant independent of whether DelBucket retries or not.

That said, given the retries, I think it's sensible and more useful for
DelBucket to stop reporting NoSuchBucket errors. I'll do that in a follow up.

[niemeyer, 2013-02-01 20:25:49]

Please take a look.

[niemeyer, 2013-02-02 18:20:16]

Please take a look.

[rog, 2013-02-04 11:09:11]

LGTM with a few minor points.

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go
File aws/attempt.go (right):

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go#newcode38
aws/attempt.go:38: if !a.force && !now.Add(sleep).Before(a.end) &&
a.strategy.Min <= a.count {
i'd find the Min test easier to read if the thing being tested was on the left
as usual (a.count > Min)

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go#newcode52
aws/attempt.go:52: sleep := a.strategy.Delay - now.Sub(a.last)
subtracting a zero time makes me feel nervous about overflow. perhaps a comment
for reassurance?

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go#newcode63
aws/attempt.go:63: if a.force || a.strategy.Min > a.count {
as above, a.count <= a.strategy.Min would seem more usual.

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go#newcode67
aws/attempt.go:67: if a.end.IsZero() || now.Add(a.nextSleep(now)).Before(a.end)
{
end can never be zero here AFAICS

https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go
File aws/attempt_test.go (right):

https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go#newcode50
aws/attempt_test.go:50: a = aws.AttemptStrategy{Total: 1e8, Min: 2}.Start()
personally i think that using e9 makes the durations easier to read (the number
to the left of the exponent is the number of seconds).

at the least we should probably be consistent throughout this file.

https://codereview.appspot.com/7264043/diff/13001/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/7264043/diff/13001/s3/s3.go#newcode537
s3/s3.go:537: case "read", "write":
i'd definitely include "remote error" here, as that's the op of the error i've
seen most ("remote error: handshake failure")

[niemeyer, 2013-02-04 12:47:09]

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go
File aws/attempt.go (right):

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go#newcode38
aws/attempt.go:38: if !a.force && !now.Add(sleep).Before(a.end) &&
a.strategy.Min <= a.count {
On 2013/02/04 11:09:11, rog wrote:
> i'd find the Min test easier to read if the thing being tested was on the left
> as usual (a.count > Min)

You say you'd find it easier and then get the inequality wrong? :-)

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go#newcode52
aws/attempt.go:52: sleep := a.strategy.Delay - now.Sub(a.last)
On 2013/02/04 11:09:11, rog wrote:
> subtracting a zero time makes me feel nervous about overflow. perhaps a
comment
> for reassurance?

I'll test for last explicitly.

https://codereview.appspot.com/7264043/diff/13001/aws/attempt.go#newcode67
aws/attempt.go:67: if a.end.IsZero() || now.Add(a.nextSleep(now)).Before(a.end)
{
On 2013/02/04 11:09:11, rog wrote:
> end can never be zero here AFAICS

Indeed, this is historical. Will drop.

https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go
File aws/attempt_test.go (right):

https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go#newcode50
aws/attempt_test.go:50: a = aws.AttemptStrategy{Total: 1e8, Min: 2}.Start()
On 2013/02/04 11:09:11, rog wrote:
> personally i think that using e9 makes the durations easier to read (the
number

The order of inequalities.. the exponent of integers.. I suggest focusing
reviews on points that are less subjective.

[rog, 2013-02-04 13:05:05]

https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go
File aws/attempt_test.go (right):

https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go#newcode50
aws/attempt_test.go:50: a = aws.AttemptStrategy{Total: 1e8, Min: 2}.Start()
On 2013/02/04 12:47:09, niemeyer wrote:
> On 2013/02/04 11:09:11, rog wrote:
> > personally i think that using e9 makes the durations easier to read (the
> number
> 
> The order of inequalities.. the exponent of integers.. I suggest focusing
> reviews on points that are less subjective.

i mention things that seem to break the flow of the code
for me. feel free to ignore. sorry.

[niemeyer, 2013-02-04 13:59:57]

On 2013/02/04 13:05:05, rog wrote:
> https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go
> File aws/attempt_test.go (right):
> 
>
https://codereview.appspot.com/7264043/diff/13001/aws/attempt_test.go#newcode50
> aws/attempt_test.go:50: a = aws.AttemptStrategy{Total: 1e8, Min: 2}.Start()
> On 2013/02/04 12:47:09, niemeyer wrote:
> > On 2013/02/04 11:09:11, rog wrote:
> > > personally i think that using e9 makes the durations easier to read (the
> > number
> > 
> > The order of inequalities.. the exponent of integers.. I suggest focusing
> > reviews on points that are less subjective.
> 
> i mention things that seem to break the flow of the code
> for me. feel free to ignore. sorry.

Understood, but realize that if I wrote it down, it means it's not an issue for
me. When reviewing code, one has to ponder whether there is a real contribution
to the project, or one is nitpicking over something highly personal and
subjective. The two issues pinpointed are clearly in the latter camp.

[niemeyer, 2013-02-04 21:33:44]

*** Submitted:

s3: introduce retrying on known errors

R=
CC=
https://codereview.appspot.com/7264043