environs/ec2: retry s3 Put requests

environs/ec2: retry s3 Put requests

This is a band-aid until the relevant functionality
is implemented in the s3 package itself.

https://code.launchpad.net/~rogpeppe/juju-core/282-ec2-retry-put/+merge/158444

(do not edit description out of merge proposal)

[dimitern, 2013-04-11 18:18:05]

Looks really good, but I can't see specific tests for the retry logic.

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode38
environs/ec2/storage.go:38: // PutBucket always return a 200 if we recreate an
existing bucket for the
s/return/returns/ ?

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode42
environs/ec2/storage.go:42: if s3ErrCode(err) == "BucketAlreadyOwnedByYou" {
put this in a const perhaps?

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode94
environs/ec2/storage.go:94: // goamz/s3.
maybe put a TODO to get rid of this once goamz makes it possible with an API
change?

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode113
environs/ec2/storage.go:113: case "InternalError", "NoSuchUpload",
"NoSuchBucket":
these as well

[rog, 2013-04-15 23:19:56]

unfortunately i can't add specific tests for the retry logic until i've added
control hooks into ec2test.

that probably scuppers this CL, but i'll propose it anyway.

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode38
environs/ec2/storage.go:38: // PutBucket always return a 200 if we recreate an
existing bucket for the
On 2013/04/11 18:18:06, dimitern wrote:
> s/return/returns/ ?

Done.

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode42
environs/ec2/storage.go:42: if s3ErrCode(err) == "BucketAlreadyOwnedByYou" {
On 2013/04/11 18:18:06, dimitern wrote:
> put this in a const perhaps?

i don't think that would be worth it currently.
if we start using elsewhere, probably.

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode94
environs/ec2/storage.go:94: // goamz/s3.
On 2013/04/11 18:18:06, dimitern wrote:
> maybe put a TODO to get rid of this once goamz makes it possible with an API
> change?

Done.

https://codereview.appspot.com/8545045/diff/1/environs/ec2/storage.go#newcode113
environs/ec2/storage.go:113: case "InternalError", "NoSuchUpload",
"NoSuchBucket":
On 2013/04/11 18:18:06, dimitern wrote:
> these as well

i'd prefer to keep this function an exact copy of the original if that's ok.

also, as above, i don't think that string constants
should necessarily have associated names.

the literals themselves can be looked up in the
amazon documentation and represent themselves quite
well.

that said, at some point it might be nice if goamz defined all the known error
codes, at which point we would certainly use them.

[rog, 2013-04-15 23:23:15]

Please take a look.

[fwereade, 2013-04-17 13:51:36]

NOT LGTM -- I don't think this special case justifies the interface change.
Please try to cast to ReadSeeker for reliable Puts, and fall back to unreliable
if it's really just a Reader.

https://codereview.appspot.com/8545045/diff/5001/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/8545045/diff/5001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:49: log.Noticef("environs/ec2: transient S3 PutBucket
error: %v", err)
Warningf -- if that. Probably Infof or even Debugf...

https://codereview.appspot.com/8545045/diff/5001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:68: log.Noticef("environs/ec2: transient S3 Put error:
%v", err)
Warningf at least