environ: add SetConfig

environ: add SetConfig

Discussion:

This proposal adds the ability for Environs to update their configuration.
This facility replaces the previously proposed proxy Environ and provides
more explicit control over when an Environs configuration is updated.

As a side effect, several mutexes had to be added to control the visibility 
of variables that could be affected by SetConfig. Sorry Roger.

Additionally, in the dummy provider, the zookeeper value is still parsed
as a configuration item but is not used. It wasn't used before either, so
I've removed it from dummy.environConfig.

https://code.launchpad.net/~dave-cheney/juju/go-environs-setconfig/+merge/106930

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-05-23 03:18:48]

Please take a look.

[rog, 2012-05-23 08:27:34]

i realise this is a WIP, but i thought i'd make some comments anyway...

i have mixed feelings about this. one the one hand, i like SetConfig - i think
it represents  the problem nicely.
on the other hand it's a bit sad to see all those mutexed accesses cluttering up
the logic.

two possible thoughts:
1) we could document that it was not safe to call SetConfig concurrently with
any other methods on Environ. that way we push the problem up to the layer
above, which may be able to solve it more easily.
2) we could use the same proxy approach as suggested before, but *inside*
environs/ec2 (i.e. ec2.environConfig.Open would return a *proxy.Environ). since
ec2.environ has no state, the result should be indistinguishable.

not really sure that either of those are better than what we've got here, but
food for thought perhaps.

https://codereview.appspot.com/6229046/diff/4001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/dummy/environs.go#n...
environs/dummy/environs.go:91: broken    bool
now that it can be set, i think it would make sense
to put the environConfig as a field in here; then there can be a single method
that guards the config, and everything can use that, e.g. if e.config().broken
...

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode48
environs/ec2/ec2.go:48: _s3     *s3.S3
i don't really see that these are necessary. ec2() and s3() could create them on
demand from the config.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode52
environs/ec2/ec2.go:52: configLock sync.Mutex
i'd group this together with the thing it's protecting.
also "mutex" is more a conventional name than "lock", so maybe "configMutex"
would be a better.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode182
environs/ec2/ec2.go:182: c, ok := cfg.(*providerConfig)
i think a panic would be fine here.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode525
environs/ec2/ec2.go:525: e.configLock.Lock()
cfg := e.config()
return ec2.New(cfg.auth, Regions[cfg.region])

and below with s3.

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

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:18: bucketMutex sync.Mutex
i'd prefer to keep the original field ordering, keeping bucketMutex above the
fields it's protecting. and i'm not keen on the underscore name, but can't think
of anything much better. bkt ?

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:102: s._bucket = bucket
this should probably set s.madeBucket to false too.

[niemeyer, 2012-05-23 19:35:54]

Thanks Dave, I quite appreciate this. Lots of comments, but almost nothing
important.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode46
environs/ec2/ec2.go:46: cfg     *providerConfig
I suggest calling this configUnlocked, ec2Unlocked, and s3Unlocked, to make it
clear on usage that touching it must be protected (e.cfg and e.config() both
look equally ok).

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode48
environs/ec2/ec2.go:48: _s3     *s3.S3
On 2012/05/23 08:27:34, rog wrote:
> i don't really see that these are necessary. ec2() and s3() could create them
on
> demand from the config.

I think caching them is fine as well, though, since it's trivial and the
environment needs them to do pretty much anything.

I'd call them something like ec2Cached and s3Cached, though, to make the
intention more clear.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode182
environs/ec2/ec2.go:182: c, ok := cfg.(*providerConfig)
On 2012/05/23 08:27:34, rog wrote:
> i think a panic would be fine here.

+1

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode190
environs/ec2/ec2.go:190: func (e *environ) setConfig(cfg *providerConfig) {
This method doesn't seem necessary. SetConfig and setConfig are the same thing
pretty much, so we can have just the public version.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode400
environs/ec2/ec2.go:400: _, err = e.ec2().TerminateInstances(ids)
This is begging for a local variable. There are two loops using it.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode467
environs/ec2/ec2.go:467: resp, err := e.ec2().CreateSecurityGroup(name, "juju
group")
Ditto.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode517
environs/ec2/ec2.go:517: func (e *environ) config() *providerConfig {
It'd be nice to have this method and the ones below closer to the rest of the
config management above (SetConfig), since they are so related.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode525
environs/ec2/ec2.go:525: e.configLock.Lock()
On 2012/05/23 08:27:34, rog wrote:
> cfg := e.config()
> return ec2.New(cfg.auth, Regions[cfg.region])
> 
> and below with s3.

Dave's logic looks fine to me.

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

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:16: _bucket *s3.Bucket
bucketUnlocked if the suggested convention looks good to you.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:23: return fmt.Errorf("cannot make S3 control bucket:
%v", err)
I know you've moved that from elsewhere, but since we're changing it, can you
please take the chance to improve the error:

"ec2: cannot make ..." (the ec2 is from the package name)

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:27: return fmt.Errorf("cannot write file %q to control
bucket: %v", file, err)
Same.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:41: s.madeBucket = true
Semantics have changed here. This variable was supposed to be protected by the
lock in the original version of the logic. I suggest putting back the logic and
(carefully) using bucketUnlocked here.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:48: r, err = s.bucket().GetReader(file)
A local variable would be nice here too.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:93: func (s *storage) bucket() *s3.Bucket {
Would also be nice to have these accessor functions closer to the struct above.

https://codereview.appspot.com/6229046/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/interface.go#newcod...
environs/interface.go:111: SetConfig(EnvironConfig) error
This interface is already inconsistent in that some parameters are named while
others are not. I suggest we name them consistently, since the name is helpful
documentation in some cases (doesn't matter much on this one):

    SetConfig(config EnvironConfig) error

[dave_cheney.net, 2012-05-23 21:26:31]

On 2012/05/23 08:27:34, rog wrote:
> i realise this is a WIP, but i thought i'd make some comments anyway...

Please comment away, the sooner this branch is in the better.
 
> i have mixed feelings about this. one the one hand, i like SetConfig - i think
> it represents  the problem nicely.
> on the other hand it's a bit sad to see all those mutexed accesses cluttering
up
> the logic.

Yes, it is regrettable.
 
> two possible thoughts:
> 1) we could document that it was not safe to call SetConfig concurrently with
> any other methods on Environ. that way we push the problem up to the layer
> above, which may be able to solve it more easily.

It's not just about documenting which methods may operate on stale values, but
ensuring that any access to a value that
may be altered is properly fenced.

> 2) we could use the same proxy approach as suggested before, but *inside*
> environs/ec2 (i.e. ec2.environConfig.Open would return a *proxy.Environ).
since
> ec2.environ has no state, the result should be indistinguishable.

That is an interesting idea, define SetConfig on the proxy Environ, then call it
externally when we want it to reload 
it's inner Environ when we choose. Gustavo, how does that sound ?

[dave_cheney.net, 2012-05-23 21:57:06]

Please take a look.

https://codereview.appspot.com/6229046/diff/4001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/dummy/environs.go#n...
environs/dummy/environs.go:91: broken    bool
On 2012/05/23 08:27:34, rog wrote:
> now that it can be set, i think it would make sense
> to put the environConfig as a field in here; then there can be a single method
> that guards the config, and everything can use that, e.g. if e.config().broken
> ...

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode48
environs/ec2/ec2.go:48: _s3     *s3.S3
On 2012/05/23 08:27:34, rog wrote:
> i don't really see that these are necessary. ec2() and s3() could create them
on
> demand from the config.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode48
environs/ec2/ec2.go:48: _s3     *s3.S3
After looking at the definition of s3.S3 and ec2.EC2 I'm in favor of not caching
them. PTAL

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode52
environs/ec2/ec2.go:52: configLock sync.Mutex
On 2012/05/23 08:27:34, rog wrote:
> i'd group this together with the thing it's protecting.
> also "mutex" is more a conventional name than "lock", so maybe "configMutex"
> would be a better.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode400
environs/ec2/ec2.go:400: _, err = e.ec2().TerminateInstances(ids)
On 2012/05/23 19:35:54, niemeyer wrote:
> This is begging for a local variable. There are two loops using it.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode467
environs/ec2/ec2.go:467: resp, err := e.ec2().CreateSecurityGroup(name, "juju
group")
Done. running short of local variable names.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode517
environs/ec2/ec2.go:517: func (e *environ) config() *providerConfig {
On 2012/05/23 19:35:54, niemeyer wrote:
> It'd be nice to have this method and the ones below closer to the rest of the
> config management above (SetConfig), since they are so related.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode525
environs/ec2/ec2.go:525: e.configLock.Lock()
On 2012/05/23 08:27:34, rog wrote:
> cfg := e.config()
> return ec2.New(cfg.auth, Regions[cfg.region])
> 
> and below with s3.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode525
environs/ec2/ec2.go:525: e.configLock.Lock()
I'm going to try it rogers way here.

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

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:16: _bucket *s3.Bucket
On 2012/05/23 19:35:54, niemeyer wrote:
> bucketUnlocked if the suggested convention looks good to you.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:23: return fmt.Errorf("cannot make S3 control bucket:
%v", err)
On 2012/05/23 19:35:54, niemeyer wrote:
> I know you've moved that from elsewhere, but since we're changing it, can you
> please take the chance to improve the error:
> 
> "ec2: cannot make ..." (the ec2 is from the package name)

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:27: return fmt.Errorf("cannot write file %q to control
bucket: %v", file, err)
On 2012/05/23 19:35:54, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:41: s.madeBucket = true
fixed

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:93: func (s *storage) bucket() *s3.Bucket {
On 2012/05/23 19:35:54, niemeyer wrote:
> Would also be nice to have these accessor functions closer to the struct
above.

Done.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/storage.go#newc...
environs/ec2/storage.go:102: s._bucket = bucket
Done, and i've indicated that madeBucket is part of the mutexes scope here.

https://codereview.appspot.com/6229046/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/interface.go#newcod...
environs/interface.go:111: SetConfig(EnvironConfig) error
This was my mistake, I misread the signature for StateInfo above.

On 2012/05/23 19:35:54, niemeyer wrote:
> This interface is already inconsistent in that some parameters are named while
> others are not. I suggest we name them consistently, since the name is helpful
> documentation in some cases (doesn't matter much on this one):
> 
>     SetConfig(config EnvironConfig) error

[niemeyer, 2012-05-23 23:27:06]

LGTM assuming at least the panic fixed.

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode525
environs/ec2/ec2.go:525: e.configLock.Lock()
On 2012/05/23 21:57:06, dfc wrote:
> I'm going to try it rogers way here.

Ok, sounds good.

https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go#newcode181
environs/ec2/ec2.go:181: panic(fmt.Sprintf("configuration was %T, expected %T",
cfg, new(providerConfig)))
The compiler can do that for us. Just use:

    c := cfg.(*providerConfig)

https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go#newcode482
environs/ec2/ec2.go:482: ectwo := e.ec2()
I suggest fooinst when we get into that kind of issue (ec2inst, in this case).

[dave_cheney.net, 2012-05-24 00:08:50]

Please take a look.

https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go#newcode181
environs/ec2/ec2.go:181: panic(fmt.Sprintf("configuration was %T, expected %T",
cfg, new(providerConfig)))
On 2012/05/23 23:27:07, niemeyer wrote:
> The compiler can do that for us. Just use:
> 
>     c := cfg.(*providerConfig)

Done.

https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go#newcode482
environs/ec2/ec2.go:482: ectwo := e.ec2()
On 2012/05/23 23:27:07, niemeyer wrote:
> I suggest fooinst when we get into that kind of issue (ec2inst, in this case).

Done.

https://codereview.appspot.com/6229046/diff/12001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6229046/diff/12001/environs/interface.go#newco...
environs/interface.go:111: SetConfig(config EnvironConfig) error
As there is no error returned by an implementing type, I have removed the return
argument.

[niemeyer, 2012-05-24 21:41:05]

Putting it back to WIP due to the introduced bug.

https://codereview.appspot.com/6229046/diff/12001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6229046/diff/12001/environs/interface.go#newco...
environs/interface.go:111: SetConfig(config EnvironConfig) error
On 2012/05/24 00:08:50, dfc wrote:
> As there is no error returned by an implementing type, I have removed the
return
> argument.

Sounds like a good idea in theory. The configuration is validated on NewConfig
anyway, so any errors should be dealt with there. We'll see how that pans out in
practice.

https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go#newcode184
environs/ec2/ec2.go:184: e.storage.setBucket(e.s3().Bucket(config.bucket))
This seems problematic. Think about two SetConfig calls going concurrently.
There's a chance that the configuration will point to one bucket, and the
storage will point to a different one, so the end result is a mixture of both
configurations.

[dave_cheney.net, 2012-05-25 00:38:56]

Patch set 8 incorporates https://codereview.appspot.com/6244053.

[dave_cheney.net, 2012-05-25 00:39:58]

Please take a look.

[rog, 2012-05-25 08:07:22]

https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go#newcode184
environs/ec2/ec2.go:184: e.storage.setBucket(e.s3().Bucket(config.bucket))
On 2012/05/24 21:41:05, niemeyer wrote:
> This seems problematic. Think about two SetConfig calls going concurrently.
> There's a chance that the configuration will point to one bucket, and the
> storage will point to a different one, so the end result is a mixture of both
> configurations.

easy fix: make the bucketMutex in the storage point to the configMutex, or make
the storage point back to the environ and use the mutex in place there.

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

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/storage.go#new...
environs/ec2/storage.go:40: defer s.bucketMutex.Unlock()
if there's no madeBucket, then there's no need for a mutex here. i'd prefer to
actually fix the logic to use madeBucket correctly (my original oops, obviously)
so that we don't incur two round-trips for every Put.

[dave_cheney.net, 2012-05-25 08:09:33]

Roger, do you have bandwidth implement makeBucket properly. I've put it on my
backlog but it won't see to the top of the stack for a while. 

On 25/05/2012, at 18:07, rogpeppe@gmail.com wrote:

> 
> https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go
> File environs/ec2/ec2.go (right):
> 
>
https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go#newcode184
> environs/ec2/ec2.go:184:
> e.storage.setBucket(e.s3().Bucket(config.bucket))
> On 2012/05/24 21:41:05, niemeyer wrote:
>> This seems problematic. Think about two SetConfig calls going
> concurrently.
>> There's a chance that the configuration will point to one bucket, and
> the
>> storage will point to a different one, so the end result is a mixture
> of both
>> configurations.
> 
> easy fix: make the bucketMutex in the storage point to the configMutex,
> or make the storage point back to the environ and use the mutex in place
> there.
> 
> https://codereview.appspot.com/6229046/diff/18004/environs/ec2/storage.go
> File environs/ec2/storage.go (right):
> 
>
https://codereview.appspot.com/6229046/diff/18004/environs/ec2/storage.go#new...
> environs/ec2/storage.go:40: defer s.bucketMutex.Unlock()
> if there's no madeBucket, then there's no need for a mutex here. i'd
> prefer to actually fix the logic to use madeBucket correctly (my
> original oops, obviously) so that we don't incur two round-trips for
> every Put.
> 
> https://codereview.appspot.com/6229046/

[rog, 2012-05-25 08:21:05]

np. i'll do it now.

On 25 May 2012 09:09, Dave Cheney <dave@cheney.net> wrote:
> Roger, do you have bandwidth implement makeBucket properly. I've put it on my
backlog but it won't see to the top of the stack for a while.
>
> On 25/05/2012, at 18:07, rogpeppe@gmail.com wrote:
>
>>
>> https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go
>> File environs/ec2/ec2.go (right):
>>
>>
https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go#newcode184
>> environs/ec2/ec2.go:184:
>> e.storage.setBucket(e.s3().Bucket(config.bucket))
>> On 2012/05/24 21:41:05, niemeyer wrote:
>>> This seems problematic. Think about two SetConfig calls going
>> concurrently.
>>> There's a chance that the configuration will point to one bucket, and
>> the
>>> storage will point to a different one, so the end result is a mixture
>> of both
>>> configurations.
>>
>> easy fix: make the bucketMutex in the storage point to the configMutex,
>> or make the storage point back to the environ and use the mutex in place
>> there.
>>
>> https://codereview.appspot.com/6229046/diff/18004/environs/ec2/storage.go
>> File environs/ec2/storage.go (right):
>>
>>
https://codereview.appspot.com/6229046/diff/18004/environs/ec2/storage.go#new...
>> environs/ec2/storage.go:40: defer s.bucketMutex.Unlock()
>> if there's no madeBucket, then there's no need for a mutex here. i'd
>> prefer to actually fix the logic to use madeBucket correctly (my
>> original oops, obviously) so that we don't incur two round-trips for
>> every Put.
>>
>> https://codereview.appspot.com/6229046/

[niemeyer, 2012-05-25 19:08:02]

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go#newcode179
environs/ec2/ec2.go:179: func (e *environ) storage() *storage {
Do we need this method? Storage has a single line that simply returns it.

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go#newcode181
environs/ec2/ec2.go:181: return &storage{bucket: e.s3().Bucket(config.bucket)}
This is still not nice. It's quite sad to be creating a S3+bucket+storage on
*every single call* that needs to use it.

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go#newcode187
environs/ec2/ec2.go:187: return nil
This diverges from the original logic. If tests still pass, we need another
test.

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go#newcode189
environs/ec2/ec2.go:189: return &storage{bucket:
e.s3().Bucket(config.publicBucket)}
Ditto.

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go#newcode208
environs/ec2/ec2.go:208: return ec2.New(cfg.auth, Regions[cfg.region])
Ditto.

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go#newcode214
environs/ec2/ec2.go:214: return s3.New(cfg.auth, Regions[cfg.region])
Ditto.

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

https://codereview.appspot.com/6229046/diff/18004/environs/ec2/storage.go#new...
environs/ec2/storage.go:40: defer s.bucketMutex.Unlock()
On 2012/05/25 08:07:22, rog wrote:
> if there's no madeBucket, then there's no need for a mutex here. i'd prefer to
> actually fix the logic to use madeBucket correctly (my original oops,
obviously)
> so that we don't incur two round-trips for every Put.

Indeed, no need for the method either.

[niemeyer, 2012-05-25 19:11:40]

I went to grab the branch to help out and noticed that you actually already
nailed down pretty much all the points I've made above. Can you please
re-propose it so we can have a look at it and hopefully finish the bikeshed?

[niemeyer, 2012-05-25 19:24:52]

Dave has replaced this proposal with:

https://codereview.appspot.com/6256050/