state: implement allWatcher.run

state: implement allWatcher.run

We also implement StateWatcher methods
to talk to the running allWatcher (currently
temporarily renamed to xStateWatcher
to avoid messing with the existing dummy
StateWatcher).

https://code.launchpad.net/~rogpeppe/juju-core/244-allwatcher-loop/+merge/153610

Requires: https://code.launchpad.net/~rogpeppe/juju-core/243-allwatcher-respond/+merge/153519

(do not edit description out of merge proposal)

[rog, 2013-03-15 17:45:11]

Please take a look.

[dimitern, 2013-03-15 18:18:00]

Very nice! LGTM with just some trivials.

https://codereview.appspot.com/7815044/diff/5001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (left):

https://codereview.appspot.com/7815044/diff/5001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:569: Err() error
this seems wrong - please, explain?

https://codereview.appspot.com/7815044/diff/5001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7815044/diff/5001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:235: ew := w.w.(*state.EntityWatcher)
yeah, it's better like this.

https://codereview.appspot.com/7815044/diff/5001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7815044/diff/5001/state/megawatcher.go#newcode92
state/megawatcher.go:92: // TODO better error?
definitely, possibly even a panic?

https://codereview.appspot.com/7815044/diff/5001/state/megawatcher.go#newcode188
state/megawatcher.go:188: // TODO(rog) distinguish between temporary and
permanent errors:
s/d/D/

https://codereview.appspot.com/7815044/diff/5001/state/megawatcher.go#newcode189
state/megawatcher.go:189: // if we get an error in loop, this logic kill the
state's allWatcher
s/kill/kills/

[rog, 2013-03-18 12:41:51]

Please take a look.

[fwereade, 2013-03-18 13:44:32]

Looking good, I think, but one significant question:

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go#newcode69
state/megawatcher.go:69: return w.all.tomb.Err()
.Wait()?

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go#newcod...
state/megawatcher.go:200: if err := aw.backing.getAll(aw.all); err != nil {
I'm not quite clear on the consequences of races between the watch() and the
getAll(). What's the worst that could happen?

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
state/megawatcher_internal_test.go:762: <-done
Timeouts on naked channel operations please :).

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
state/megawatcher_internal_test.go:797: case <-time.After(1 * time.Second):
Do we need quite this long?

[fwereade, 2013-03-18 13:48:12]

LGTM

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go#newcod...
state/megawatcher.go:200: if err := aw.backing.getAll(aw.all); err != nil {
On 2013/03/18 13:44:33, fwereade wrote:
> I'm not quite clear on the consequences of races between the watch() and the
> getAll(). What's the worst that could happen?

Please add a comment explaining why both double-add and double-remove are safe

[rog, 2013-03-18 15:34:53]

Please take a look.

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go#newcode69
state/megawatcher.go:69: return w.all.tomb.Err()
On 2013/03/18 13:44:33, fwereade wrote:
> .Wait()?

we know the tomb is already dead, so Err seems like the right thing here, no?

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
state/megawatcher_internal_test.go:762: <-done
On 2013/03/18 13:44:33, fwereade wrote:
> Timeouts on naked channel operations please :).

it's waiting for waitNext, which already has a timeout, so i don't believe
another one is necessary.

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
state/megawatcher_internal_test.go:797: case <-time.After(1 * time.Second):
On 2013/03/18 13:44:33, fwereade wrote:
> Do we need quite this long?

i don't think it matters. this only happens if the test fails.

[fwereade, 2013-03-18 15:38:41]

Still LGTM :)

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher.go#newcode69
state/megawatcher.go:69: return w.all.tomb.Err()
On 2013/03/18 15:34:53, rog wrote:
> On 2013/03/18 13:44:33, fwereade wrote:
> > .Wait()?
> 
> we know the tomb is already dead, so Err seems like the right thing here, no?

D'oh.

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
state/megawatcher_internal_test.go:762: <-done
On 2013/03/18 15:34:53, rog wrote:
> On 2013/03/18 13:44:33, fwereade wrote:
> > Timeouts on naked channel operations please :).
> 
> it's waiting for waitNext, which already has a timeout, so i don't believe
> another one is necessary.

Ah, good point.

https://codereview.appspot.com/7815044/diff/13001/state/megawatcher_internal_...
state/megawatcher_internal_test.go:797: case <-time.After(1 * time.Second):
On 2013/03/18 15:34:53, rog wrote:
> On 2013/03/18 13:44:33, fwereade wrote:
> > Do we need quite this long?
> 
> i don't think it matters. this only happens if the test fails.

I'd still favour dropping it to something a bit tighter -- 500ms is plenty,
surely, and standard elsewhere.

[rog, 2013-03-18 15:58:17]

*** Submitted:

state: implement allWatcher.run

We also implement StateWatcher methods
to talk to the running allWatcher (currently
temporarily renamed to xStateWatcher
to avoid messing with the existing dummy
StateWatcher).

R=dimitern, fwereade
CC=
https://codereview.appspot.com/7815044