add presence.ChildrenWatcher

add presence.ChildrenWatcher

This mimics watcher.ChildrenWatcher, but expects that all children be
presence nodes; and notifies of child node Pinger life/death, rather
than mere existence of child nodes.

Note that there is still a node deletion detection race between AliveW and
the watcher.ChildrenWatcher, but it's a lot easier to deal with in this
context than it was when I was trying to solve the same issue with a
RelatedUnitsWatcher using multiple relationUnitWatcher~s.

https://code.launchpad.net/~fwereade/juju-core/watch-presence-children/+merge/112146

(do not edit description out of merge proposal)

[fwereade, 2012-06-26 15:39:55]

Please take a look.

[fwereade, 2012-06-26 15:58:59]

Please take a look.

[niemeyer, 2012-06-27 05:41:57]

Neat. Seems like a good abstraction.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:285: // ChildrenWatcher mimics
state.watcher.ChildrenWatcher, but treats nodes that
state.watcher.ChildrenWatcher => state/watcher's ChildrenWatcher

That dotted notation doesn't exist in Go.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:336: defer func() {
There's no reason for this to be a closure. defer w.finish() or similar would
work.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:347: close(w.changes)
Closing these channels seems to just create extra overhead for handling below. I
suspect keeping them unclosed will make the logic simpler without introducing
issues.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:372: panic("updates channel closed unexpectedly")
If closing it can only lead to panics, we don't need to close it.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:397: close(stop)
What if we discarded delete notification for nodes coming from the children
watcher, and allowed the presence watcher itself to handle all deletions?
Wouldn't that suppress the need for all those stops, and avoid the race as well?

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:427: case <-stop:
If the above suggestion works, it doesn't feel like we need this. We need to
track whether they're running with wg so we can wait, but this can also monitor
the same tomb as a termination flag.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:431: w.tomb.Killf("presence watch closed
unexpectedly")
Watchers can close channels due to a number of reasons, including network
errors. This shouldn't be assuming it's unexpected to get an error from the
watcher.

Also, it's not returning.

[fwereade, 2012-06-27 14:52:51]

Hmm, I appear to have got tangled: deleting MP, recreating with new prereq. Will
post a link to the new CL when I know what it is.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:285: // ChildrenWatcher mimics
state.watcher.ChildrenWatcher, but treats nodes that
On 2012/06/27 05:41:57, niemeyer wrote:
> state.watcher.ChildrenWatcher => state/watcher's ChildrenWatcher
> 
> That dotted notation doesn't exist in Go.

Good point :). Done.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:336: defer func() {
On 2012/06/27 05:41:57, niemeyer wrote:
> There's no reason for this to be a closure. defer w.finish() or similar would
> work.

Changed enough that Doneness is moot :).

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:347: close(w.changes)
On 2012/06/27 05:41:57, niemeyer wrote:
> Closing these channels seems to just create extra overhead for handling below.
I
> suspect keeping them unclosed will make the logic simpler without introducing
> issues.

Done.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:372: panic("updates channel closed unexpectedly")
On 2012/06/27 05:41:57, niemeyer wrote:
> If closing it can only lead to panics, we don't need to close it.

Done.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:397: close(stop)
On 2012/06/27 05:41:57, niemeyer wrote:
> What if we discarded delete notification for nodes coming from the children
> watcher, and allowed the presence watcher itself to handle all deletions?
> Wouldn't that suppress the need for all those stops, and avoid the race as
well?

Very neat.

/me removes hat.

Done.

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:427: case <-stop:
On 2012/06/27 05:41:57, niemeyer wrote:
> If the above suggestion works, it doesn't feel like we need this. We need to
> track whether they're running with wg so we can wait, but this can also
monitor
> the same tomb as a termination flag.

I don't think we even need wg any more :).

https://codereview.appspot.com/6335057/diff/3001/state/presence/presence.go#n...
state/presence/presence.go:431: w.tomb.Killf("presence watch closed
unexpectedly")
On 2012/06/27 05:41:57, niemeyer wrote:
> Watchers can close channels due to a number of reasons, including network
> errors. This shouldn't be assuming it's unexpected to get an error from the
> watcher.

Tweaked wording. The proper solution, I think, is to have AliveW watches return
events instead of just bools; definitely one for another CL.

> Also, it's not returning.

Whoops. Done.

[fwereade, 2012-06-27 14:53:58]

Please take a look.

[fwereade, 2012-06-27 14:55:48]

On 2012/06/27 14:53:58, fwereade wrote:
> Please take a look.

Hmm.

OK, I'm going to try harder with this deletion business :/.

[fwereade, 2012-06-27 15:01:29]

On 2012/06/27 14:55:48, fwereade wrote:
> On 2012/06/27 14:53:58, fwereade wrote:
> > Please take a look.
> 
> Hmm.
> 
> OK, I'm going to try harder with this deletion business :/.

Continued at: https://codereview.appspot.com/6354045