Reduce unnecessary s3 timeouts

Reduce unnecessary s3 timeouts

This branch deals with the issue of Get() and List() retries
being done separately in different places. The s3 library
always retries regardless of whether such an action is
necessary, and the ec2 provider also has its own retry
mechanism. Both are unnecessary. The retry logic belongs
at the application layer level where a sensible decision
can be made as to how and when to do it. 

New environs helper methods are introduced:
- Get() and GetDefault()
- List() and ListDefault()
I wanted to have these defined in an environs.storage
package but because of the (unnecessary) definition of
*all* the environs interfaces in the top level environs
package I couldn't do it due to import loops.

Live ec2 tests pass. The next step is to build on this work
to make the tools querying code smarter at bootstrap time.

https://code.launchpad.net/~wallyworld/juju-core/reduce-timeouts/+merge/185988

(do not edit description out of merge proposal)

[wallyworld, 2013-09-17 08:59:24]

Please take a look.

[thumper, 2013-09-18 02:50:55]

https://codereview.appspot.com/13632056/diff/1/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/13632056/diff/1/cmd/juju/upgradejuju_test.go#n...
cmd/juju/upgradejuju_test.go:364: r, err :=
environs.DefaultGet(s.Conn.Environ.Storage(), envtools.StorageName(vers))
Hmm... this line took me quite a while to parse and I'm wondering if we can make
it more obvious.

Care to add an extra temporary variable for clarity?

toolsFilename := envtools.StorageName(vers)
r, err := environs.DefaultGet(s.Conn.Environ.Storage(), toolsFilename)

I also found the name "DefaultGet" took some thinking too, as the name is
attached to environs, and not any storage type name.  It had me thinking "what
are we getting from where?"

I don't have an answer yet, perhaps something will come to me later in the
review.

https://codereview.appspot.com/13632056/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13632056/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:99: _, err = provider.LoadState(storage)
w00t, much nicer.

Since we are not messing around with err inside a loop any more, you could
delete the "var err error" three lines up and just use := here.

https://codereview.appspot.com/13632056/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13632056/diff/1/environs/interface.go#newcode86
environs/interface.go:86: DefaultConsistencyStrategy() utils.AttemptStrategy
You have changed the method name but not changed the comment above.

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

https://codereview.appspot.com/13632056/diff/1/environs/storage.go#newcode36
environs/storage.go:36: func DefaultGet(stor StorageReader, name string)
(io.ReadCloser, error) {
From the description, and reading the earlier code, I'm wondering if a more
descriptive name like:

GetFromStorage would be better than DefaultGet

and

GetFromStorageWithStrategy then Get

Similarly for List, thoughts?

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

https://codereview.appspot.com/13632056/diff/1/provider/ec2/storage.go#newcode17
provider/ec2/storage.go:17: "net"
this should go with the top block of standard libraries

[wallyworld, 2013-09-18 07:29:16]

Please take a look.

https://codereview.appspot.com/13632056/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13632056/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:99: _, err = provider.LoadState(storage)
On 2013/09/18 02:50:55, thumper wrote:
> w00t, much nicer.
> 
> Since we are not messing around with err inside a loop any more, you could
> delete the "var err error" three lines up and just use := here.

Done.

https://codereview.appspot.com/13632056/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13632056/diff/1/environs/interface.go#newcode86
environs/interface.go:86: DefaultConsistencyStrategy() utils.AttemptStrategy
On 2013/09/18 02:50:55, thumper wrote:
> You have changed the method name but not changed the comment above.

Done.

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

https://codereview.appspot.com/13632056/diff/1/environs/storage.go#newcode36
environs/storage.go:36: func DefaultGet(stor StorageReader, name string)
(io.ReadCloser, error) {
On 2013/09/18 02:50:55, thumper wrote:
> From the description, and reading the earlier code, I'm wondering if a more
> descriptive name like:
> 
> GetFromStorage would be better than DefaultGet
> 
> and
> 
> GetFromStorageWithStrategy then Get
> 
> Similarly for List, thoughts?

I bit the bullet and moved the StorageReader and StorageWriter interfaces to the
storage package. The gives the methods a good namespace. And I renamed
DefaultFoo to FooWithDefaultRetry

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

https://codereview.appspot.com/13632056/diff/1/provider/ec2/storage.go#newcode17
provider/ec2/storage.go:17: "net"
On 2013/09/18 02:50:55, thumper wrote:
> this should go with the top block of standard libraries

Done.

[wallyworld, 2013-09-18 07:30:45]

Please take a look.

[thumper, 2013-09-18 22:10:48]

A big +1 on the interface move.

I'm happy to give this a LGTM, but I want you to consider this:

There are 39 calls in the code to storage.GetWithDefaultRetry and 3 to
storage.Get.  A similar ratio exists for the List variation.

I'd like you to consider
  GetWithDefaultRetry -> Get
  Get -> GetWithRetry

https://codereview.appspot.com/13632056/diff/11001/environs/tools/tools_test.go
File environs/tools/tools_test.go (right):

https://codereview.appspot.com/13632056/diff/11001/environs/tools/tools_test....
environs/tools/tools_test.go:83: storage :=
s.env.PublicStorage().(storage.Storage)
I don't think this will work. Or if it does work, it is slightly confusing.  If
Go used a different namespace separator from a method call, there would never be
confusion between variables and packages.

[wallyworld, 2013-09-18 22:55:02]

On 2013/09/18 22:10:48, thumper wrote:
> A big +1 on the interface move.
> 
> I'm happy to give this a LGTM, but I want you to consider this:
> 
> There are 39 calls in the code to storage.GetWithDefaultRetry and 3 to
> storage.Get.  A similar ratio exists for the List variation.
> 
> I'd like you to consider
>   GetWithDefaultRetry -> Get
>   Get -> GetWithRetry
> 

Ok. Will change.

[wallyworld, 2013-09-18 22:55:51]

https://codereview.appspot.com/13632056/diff/11001/environs/tools/tools_test.go
File environs/tools/tools_test.go (right):

https://codereview.appspot.com/13632056/diff/11001/environs/tools/tools_test....
environs/tools/tools_test.go:83: storage :=
s.env.PublicStorage().(storage.Storage)
On 2013/09/18 22:10:49, thumper wrote:
> I don't think this will work. Or if it does work, it is slightly confusing. 
If
> Go used a different namespace separator from a method call, there would never
be
> confusion between variables and packages.

Ah, I already changed a bunch of these. I missed some. Thanks, will fix. I also
found a few more.