s3: expose RetryAttempts

s3: expose RetryAttempts

By doing this, we allow test code external to
goamz to adjust the timeout duration. It's particularly
useful when running against s3test - by changing
this we can reduce the time it takes to test
launchpad.net/juju-core/environs/ec2 from
3 minutes to 6 seconds.

https://code.launchpad.net/~rogpeppe/goamz/expose-retry/+merge/180099

(do not edit description out of merge proposal)

[rog, 2013-08-14 11:22:26]

Please take a look.

[dimitern, 2013-08-14 11:24:34]

Hmm.. I actually prefer the other solution - this seems less clean, with the
warning "do not set this while not testing", etc.

[jameinel, 2013-08-14 11:25:49]

I'm not a big fan of exposing a raw Boolean global variable that would end up
being touched in external test code (since we shouldn't be touching it in
external production code).

I *think* the discussion we had last time was that we could put a boolean Retry
into the s3 constructor. That seems like it would reasonably approximate what
you have, does it sound tasteful to you?

[niemeyer, 2013-08-14 13:33:19]

https://codereview.appspot.com/12740044/diff/3001/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/12740044/diff/3001/s3/s3.go#newcode54
s3/s3.go:54: var Retry = true
That sounds like a more reasonable approach (see comments in the other CL), but
it should be in a function so we're able to react to changes, and I'd also have
it in a more clear function name, such as RetryAttempts(bool).

Also, please drop the note on "testing purposes only". If we're claiming we
support it, people can do whatever they want with it.

https://codereview.appspot.com/12740044/diff/3001/s3/s3.go#newcode62
s3/s3.go:62: func attempts() aws.AttemptStrategy {
We don't need this function, or further changes in the naming convention. We can
change the attempts variable when RetryAttempts is called.

[rog, 2013-08-15 13:06:53]

https://codereview.appspot.com/12740044/diff/3001/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/12740044/diff/3001/s3/s3.go#newcode62
s3/s3.go:62: func attempts() aws.AttemptStrategy {
On 2013/08/14 13:33:19, niemeyer wrote:
> We don't need this function, or further changes in the naming convention. We
can
> change the attempts variable when RetryAttempts is called.

What do you think about thread safety with respect to a
RetryAttempts function? When it's a global variable,
it's obvious that access isn't thread safe. For functions
that mutate global state, I try to make them thread
safe in general, so I'd be tempted to put a mutex
around the variable changed by RetryAttempts
(which would probably imply that we'd still want
a function to return it).

Alternatively we could just document that it's
not safe to use with any outstanding calls in progress.

I'm happy going in either direction. Pending your response,
I'll propose the latter.

[rog, 2013-08-15 13:18:39]

Please take a look.

[niemeyer, 2013-08-15 14:28:02]

LGTM, thanks Rog.

https://codereview.appspot.com/12740044/diff/11001/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/12740044/diff/11001/s3/s3.go#newcode53
s3/s3.go:53: defaultAttempts = aws.AttemptStrategy{
Being able to do this out of order is so great.

https://codereview.appspot.com/12740044/diff/11001/s3/s3.go#newcode61
s3/s3.go:61: // retried to cope with eventual consistency. It should
... eventual consistency or temporary failures.

https://codereview.appspot.com/12740044/diff/11001/s3/s3.go#newcode62
s3/s3.go:62: // not be called while any operations are in progress.
s/any//

[rog, 2013-08-15 15:35:53]

*** Submitted:

s3: expose RetryAttempts

By doing this, we allow test code external to
goamz to adjust the timeout duration. It's particularly
useful when running against s3test - by changing
this we can reduce the time it takes to test
launchpad.net/juju-core/environs/ec2 from
3 minutes to 6 seconds.

R=dimitern, jameinel, niemeyer
CC=
https://codereview.appspot.com/12740044

https://codereview.appspot.com/12740044/diff/11001/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/12740044/diff/11001/s3/s3.go#newcode61
s3/s3.go:61: // retried to cope with eventual consistency. It should
On 2013/08/15 14:28:02, niemeyer wrote:
> ... eventual consistency or temporary failures.

Done.

https://codereview.appspot.com/12740044/diff/11001/s3/s3.go#newcode62
s3/s3.go:62: // not be called while any operations are in progress.
On 2013/08/15 14:28:02, niemeyer wrote:
> s/any//

Done.