add relationUnitsWatcher

add relationUnitsWatcher

...which expresses changes to a relation from a given unit's perspective, in
terms that I believe will be helpful for the unit agent.

In particular, the main event type produced has Changed and Departed fields,
under the rationale that the UA will itself be keeping track of membership
as it does in the python version; and that therefore it is actually more
helpful to expect it to pay attention only to -changed and -departed events,
and to automatically run -joined hooks *before* running -changed hooks in
response to unrecognised units (rather than spearately tracking all 3 kinds
of event, and *also* inserting -changed events immediately after -joineds,
as is currently done in the python).

The intent is to keep this type internal, and use it only within a
(tentatively-named) RelationUnit type, which (1) exposes the
relationUnitsWatcher's Changes channel and (2) maintains a Pinger
signalling its own presence in the relation; I'm expecting it to
look something like:

func (*Unit) Join(*Relation) (*RelationUnit, error)

func (*RelationUnit) Changes() <-chan RelationUnitsChange
func (*RelationUnit) Depart() error
func (*RelationUnit) Abscond() error

...in which Abscond seems to me to be the best available term for "depart
quietly, abandoning rather than deleting the presence node, in the hope that
departure will not be noticed by the other participants and the UA process
will have time to restart and reoccupy the node without any other UA
noticing".

This will broadly mirror the behaviour of the UnitRelationState in python,
but is IMO somewhat clearer (apart from anything else, departure is now
explicit, rather than happening magically at process exit). YMMV; please
let me know if it does, and how I could do it better :-).

https://code.launchpad.net/~fwereade/juju-core/relation-units-watcher/+merge/115001

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

(do not edit description out of merge proposal)

[fwereade, 2012-07-14 21:39:01]

Please take a look.

[niemeyer, 2012-07-18 01:44:06]

Very happy with the overall thing. Thanks for persisting on this.

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go
File state/relation_internal_test.go (right):

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go...
state/relation_internal_test.go:109: _, err :=
s.st.zk.Create("/some-scope-path", "", 0, zkPermAll)
Can't we test this in terms of the public API? It feels like we have most of the
pieces in place for that. Having this as an internal test means that it will be
difficult to refactor, and most importantly will make porting to mstate yet more
painful.

https://codereview.appspot.com/6405044/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode748
state/watcher.go:748: change = RelationUnitsChange{
Shouldn't we consider w.ignore here as well?

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode800
state/watcher.go:800: func (w *relationUnitsWatcher) updateWatches(ch
watcher.ChildrenChange) (change RelationUnitsChange, err error) {
I suggest returning RelationUnitsChange as a pointer instead. My guess is that
the error handling below will look nicer by returning nil than it looks by
instantiating a new value every other if block.

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode806
state/watcher.go:806: // lest its latest change (potentially waiting to be sent
on the updates
I've just learned a new word. Thanks. ;-)

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode812
state/watcher.go:812: if err := t.Wait(); err != nil {
I'm tempted to suggest we don't care about this error in the current context.
What do you think?

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode826
state/watcher.go:826: // N added presence nodes where N != 1.
I'd say that the point of creating it *lazily* is that there's no reason to read
the topology at all when (N - w.ignored) == 0. The reason to read it *once* is
what is explained above.

Logic looks good, either way.

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode837
state/watcher.go:837: // changes will be received on the unitLoop goroutine and
sent to
Is there a reason to not let unitLoop handle it all? Feels a bit like we have
the same algorithm twice in different incarnations.

[fwereade, 2012-07-18 10:31:16]

Please take a look.

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go
File state/relation_internal_test.go (right):

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go...
state/relation_internal_test.go:109: _, err :=
s.st.zk.Create("/some-scope-path", "", 0, zkPermAll)
On 2012/07/18 01:44:06, niemeyer wrote:
> Can't we test this in terms of the public API? It feels like we have most of
the
> pieces in place for that. Having this as an internal test means that it will
be
> difficult to refactor, and most importantly will make porting to mstate yet
more
> painful. 

I agree with everything you've said, but the public API for testing this doesn't
exist yet: that's in the followup branch, which *does* include detailed tests
for the same thing. I didn't want to include all that in this CL, because this
change is nicely self-contained, but I don't think it's simple enough to get an
LGTM without tests ;).

How would you feel if I dropped these tests in the followup branch, which does
test all this in terms of the public API?

https://codereview.appspot.com/6405044/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode748
state/watcher.go:748: change = RelationUnitsChange{
On 2012/07/18 01:44:06, niemeyer wrote:
> Shouldn't we consider w.ignore here as well?

Don't think so: we don't even start a watcher on an ignored node, so we
shouldn't get anything coming through updates.

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode800
state/watcher.go:800: func (w *relationUnitsWatcher) updateWatches(ch
watcher.ChildrenChange) (change RelationUnitsChange, err error) {
On 2012/07/18 01:44:06, niemeyer wrote:
> I suggest returning RelationUnitsChange as a pointer instead. My guess is that
> the error handling below will look nicer by returning nil than it looks by
> instantiating a new value every other if block.

SGTM. Done.

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode812
state/watcher.go:812: if err := t.Wait(); err != nil {
On 2012/07/18 01:44:06, niemeyer wrote:
> I'm tempted to suggest we don't care about this error in the current context.
> What do you think?

*I* think it's still an error and should be treated as such, but I accept that
this type will still work just fine if we do get the odd error on
settings-watcher-stoppage. So, I guess, we *could* just log it and move on...
but I'd still prefer to error out.

Are you uncertain, or hoping to convince me through Socratic dialogue? :)

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode826
state/watcher.go:826: // N added presence nodes where N != 1.
On 2012/07/18 01:44:06, niemeyer wrote:
> I'd say that the point of creating it *lazily* is that there's no reason to
read
> the topology at all when (N - w.ignored) == 0. The reason to read it *once* is
> what is explained above.
> 
> Logic looks good, either way.

Good point, reworded.

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode837
state/watcher.go:837: // changes will be received on the unitLoop goroutine and
sent to
On 2012/07/18 01:44:06, niemeyer wrote:
> Is there a reason to not let unitLoop handle it all? Feels a bit like we have
> the same algorithm twice in different incarnations.

This is necessary if we want the initial event to contain a snapshot of the
current state, which I'm pretty sure we do.

We could plausibly do something a little more like the python where we append
-changeds to -joineds, and fiddle around on -changeds to determine whether
they're real changes, but I think it's much neater for this type to give out all
the state we need and for the client (which has to maintain state anyway so it
has a diff source for process bounces) to always prepend -joineds to unknown
-changeds (and to always be able to treat -changeds as real). Sane?

I don't think that factoring out newUnitSettings(ch watcher.ContentChange)
(UnitSettings, error) is really worth the effort, but I'll do it if you like :).

[niemeyer, 2012-07-18 19:08:23]

LGTM!

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go
File state/relation_internal_test.go (right):

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go...
state/relation_internal_test.go:109: _, err :=
s.st.zk.Create("/some-scope-path", "", 0, zkPermAll)
On 2012/07/18 10:31:16, fwereade wrote:
> How would you feel if I dropped these tests in the followup branch, which does
> test all this in terms of the public API?

Okay, if you say it's actually tested entirely through the public API in the
follow up, I actually don't mind having this here for as long as it doesn't
bother us. Can you please just add a comment to the top of the function.
Something resembling:

// The logic exercised by this test is actually verified
// through the public API as well in Test<name>.

https://codereview.appspot.com/6405044/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode812
state/watcher.go:812: if err := t.Wait(); err != nil {
On 2012/07/18 10:31:16, fwereade wrote:
> Are you uncertain, or hoping to convince me through Socratic dialogue? :)

It was a non-pretentious question. :-)

https://codereview.appspot.com/6405044/diff/1/state/watcher.go#newcode837
state/watcher.go:837: // changes will be received on the unitLoop goroutine and
sent to
On 2012/07/18 10:31:16, fwereade wrote:
> On 2012/07/18 01:44:06, niemeyer wrote:
> > Is there a reason to not let unitLoop handle it all? Feels a bit like we
have
> > the same algorithm twice in different incarnations.
> 
> This is necessary if we want the initial event to contain a snapshot of the
> current state, which I'm pretty sure we do.
> 
> We could plausibly do something a little more like the python where we append
> -changeds to -joineds, and fiddle around on -changeds to determine whether
> they're real changes, but I think it's much neater for this type to give out
all
> the state we need and for the client (which has to maintain state anyway so it
> has a diff source for process bounces) to always prepend -joineds to unknown
> -changeds (and to always be able to treat -changeds as real). Sane?
> 
> I don't think that factoring out newUnitSettings(ch watcher.ContentChange)
> (UnitSettings, error) is really worth the effort, but I'll do it if you like
:).

No, thanks a lot. All sounds sane.

[fwereade, 2012-07-19 07:44:24]

*** Submitted:

add relationUnitsWatcher

...which expresses changes to a relation from a given unit's perspective, in
terms that I believe will be helpful for the unit agent.

In particular, the main event type produced has Changed and Departed fields,
under the rationale that the UA will itself be keeping track of membership
as it does in the python version; and that therefore it is actually more
helpful to expect it to pay attention only to -changed and -departed events,
and to automatically run -joined hooks *before* running -changed hooks in
response to unrecognised units (rather than spearately tracking all 3 kinds
of event, and *also* inserting -changed events immediately after -joineds,
as is currently done in the python).

The intent is to keep this type internal, and use it only within a
(tentatively-named) RelationUnit type, which (1) exposes the
relationUnitsWatcher's Changes channel and (2) maintains a Pinger
signalling its own presence in the relation; I'm expecting it to
look something like:

func (*Unit) Join(*Relation) (*RelationUnit, error)

func (*RelationUnit) Changes() <-chan RelationUnitsChange
func (*RelationUnit) Depart() error
func (*RelationUnit) Abscond() error

...in which Abscond seems to me to be the best available term for "depart
quietly, abandoning rather than deleting the presence node, in the hope that
departure will not be noticed by the other participants and the UA process
will have time to restart and reoccupy the node without any other UA
noticing".

This will broadly mirror the behaviour of the UnitRelationState in python,
but is IMO somewhat clearer (apart from anything else, departure is now
explicit, rather than happening magically at process exit). YMMV; please
let me know if it does, and how I could do it better :-).

R=niemeyer
CC=
https://codereview.appspot.com/6405044

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go
File state/relation_internal_test.go (right):

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go...
state/relation_internal_test.go:109: _, err :=
s.st.zk.Create("/some-scope-path", "", 0, zkPermAll)
On 2012/07/18 19:08:24, niemeyer wrote:
> On 2012/07/18 10:31:16, fwereade wrote:
> > How would you feel if I dropped these tests in the followup branch, which
does
> > test all this in terms of the public API?
> 
> Okay, if you say it's actually tested entirely through the public API in the
> follow up, I actually don't mind having this here for as long as it doesn't
> bother us. Can you please just add a comment to the top of the function.
> Something resembling:
> 
> // The logic exercised by this test is actually verified
> // through the public API as well in Test<name>.

Hmm, with the RelationHandler suggestion, I'm not sure precisely where it will
be tested any more. But I'll add the comment in the followup when I know where
it is :).