Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(539)

Issue 8545045: environs/ec2: retry s3 Put requests

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 7 months ago by rog
Modified:
8 years, 7 months ago
Reviewers:
dimitern, mp+158444, fwereade
Visibility:
Public.

Description

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)

Patch Set 1 #

Total comments: 8

Patch Set 2 : environs/ec2: retry s3 Put requests #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+94 lines, -32 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/synctools.go View 1 chunk +1 line, -1 line 0 comments Download
M cmd/juju/synctools_test.go View 3 chunks +2 lines, -2 lines 0 comments Download
M environs/dummy/storage.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/ec2/export_test.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/ec2/local_test.go View 3 chunks +2 lines, -3 lines 0 comments Download
M environs/ec2/state.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/ec2/storage.go View 1 4 chunks +75 lines, -16 lines 2 comments Download
M environs/interface.go View 1 chunk +3 lines, -1 line 0 comments Download
M environs/jujutest/livetests.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/jujutest/tests.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/local/storage.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/local/storage_test.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/openstack/state.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/openstack/storage.go View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4
dimitern
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 ...
8 years, 7 months ago (2013-04-11 18:18:05 UTC) #1
rog
unfortunately i can't add specific tests for the retry logic until i've added control hooks ...
8 years, 7 months ago (2013-04-15 23:19:56 UTC) #2
rog
Please take a look.
8 years, 7 months ago (2013-04-15 23:23:15 UTC) #3
fwereade
8 years, 7 months ago (2013-04-17 13:51:36 UTC) #4
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b