add unitRelationWatcher/unitRelationChange types

add unitRelationWatcher/unitRelationChange types

In python, we track settings node version rather than settings node content
as we do here. This has been changed because we want to be able to run hooks
in the contexts that caused those specific hooks to run -- including
settings for all relation members at that point in time; and this
consequently frees us from having to worry about what happens when a hook
tries to get the settings of a unit relation that *was* joined when the
hook was executed but has subsequently been deleted.  It's also beneficial
in that it only communicates changes when the actual content has changed,
rather than every time the node is written.

Note that it will still be perectly possible to implement a hook scheduler
that collapses multiple queued events from the same unit, as is done in
python, so we shouldn't be in danger of needing to store a ridiculous excess
of historical events.

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

(do not edit description out of merge proposal)

[fwereade, 2012-06-11 12:17:16]

Please take a look.

[niemeyer, 2012-06-11 21:02:50]

Good work polishing the problem up onto a nice piece of the puzzle, thanks.

Some details for consideration:

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

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode416
state/watcher.go:416: // unitRelationChange describes the state of a unit
relation.
I'm not entirely sure about it, but what do you think of inverting the whole
nomenclature, from unitRelation*, to relationUnit*?

For example:

// relationUnitChange represents the state of a unit participation
// in a relation.

The reason for the suggested change is that what we really have is a *relation*,
and the *participation* of units in it. To clarify: we don't have 10 unit
relations. We have 10 units in a single relation.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode423
state/watcher.go:423: // particular unit relation.
// relationUnitWatcher produces notifications regarding
// the participation of a unit in a particular relation.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode434
state/watcher.go:434: func newUnitRelationWatcher(zk *zookeeper.Conn, basePath,
key string, role RelationRole) *unitRelationWatcher {
Needs docs, and perhaps better parameter names (basePath points to what? key of
relation or unit?).

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode449
state/watcher.go:449: // applicable, the current version of the unit relation's
settings.
This will also benefit from the change in wording:

// Changes returns a channel that will receive notifications
// about changes in the participation of the unit in the
// watched relation. Note that multiple changes may be observed
// as a single event in the channel.
// The first event on the channel holds the presence and, if
// applicable, the current settings for the unit in the relation.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode468
state/watcher.go:468: // loop is the backend that watches a presence node and
(sometimes) a settings node.
s/(sometimes)//

or expand on what it means, otherwise we have that taste that we're really
missing something tricky.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode483
state/watcher.go:483: return
Must kill the tomb with an error. This is an abnormal termination.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode488
state/watcher.go:488: return
Ditto.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode513
state/watcher.go:513: return
return nil, err

There are exceptions, but in most cases it's good practice to send a zero value
when an error happens, so that the call site is unable to ignore the error.

Hmm.. you probably know that, though, since it was handled well below.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode528
state/watcher.go:528: return nil, tomb.ErrDying
This is bogus, and may cause the tomb to actually panic. You got a note from an
underlying tomb that it closed for whatever reason. This may be an internal
error. The tomb responsible for the goroutine that this function is under,
though, isn't necessarily dying yet. Doing something like
tomb.Kill(tomb.ErrDying) will cause a panic if the tomb isn't already dying
(because it can tell it's a lie, which indicates a logical mistake).

The right thing to do is to fmt.Errorf an appropriate note given the context.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode535
state/watcher.go:535: // The presence node is absent; send a notification.
If we got to this stage, it means the presence indicates the unit is not alive.
It may be absent or not. It's a subtle difference, but might make it easier to
read this logic when we all forget about it. :)

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode553
state/watcher.go:553: // updateSettings sends a change event indicating that the
unit relation is
Also need a touch on wording. For example, "the unit is present in the relation"
(a "unit relation" can't be present).

[fwereade, 2012-06-12 09:18:18]

Please take a look.

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

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode416
state/watcher.go:416: // unitRelationChange describes the state of a unit
relation.
On 2012/06/11 21:02:50, niemeyer wrote:
> I'm not entirely sure about it, but what do you think of inverting the whole
> nomenclature, from unitRelation*, to relationUnit*?
> 
> For example:
> 
> // relationUnitChange represents the state of a unit participation
> // in a relation.
> 
> The reason for the suggested change is that what we really have is a
*relation*,
> and the *participation* of units in it. To clarify: we don't have 10 unit
> relations. We have 10 units in a single relation.

Great suggestion, thanks.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode423
state/watcher.go:423: // particular unit relation.
On 2012/06/11 21:02:50, niemeyer wrote:
> // relationUnitWatcher produces notifications regarding
> // the participation of a unit in a particular relation.

Done.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode434
state/watcher.go:434: func newUnitRelationWatcher(zk *zookeeper.Conn, basePath,
key string, role RelationRole) *unitRelationWatcher {
On 2012/06/11 21:02:50, niemeyer wrote:
> Needs docs, and perhaps better parameter names (basePath points to what? key
of
> relation or unit?).

Done.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode468
state/watcher.go:468: // loop is the backend that watches a presence node and
(sometimes) a settings node.
On 2012/06/11 21:02:50, niemeyer wrote:
> s/(sometimes)//
> 
> or expand on what it means, otherwise we have that taste that we're really
> missing something tricky.

Done.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode483
state/watcher.go:483: return
On 2012/06/11 21:02:50, niemeyer wrote:
> Must kill the tomb with an error. This is an abnormal termination.

Heh, I thought it was funny, but this pattern is somewhat widespread and I just
followed it and deferred figuring out why it was ok; turns out it's not, and
everything makes a lot more sense. I'll propose a separate CL clearing up the
similar cases...

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode488
state/watcher.go:488: return
On 2012/06/11 21:02:50, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode513
state/watcher.go:513: return
On 2012/06/11 21:02:50, niemeyer wrote:
> return nil, err
> 
> There are exceptions, but in most cases it's good practice to send a zero
value
> when an error happens, so that the call site is unable to ignore the error.
> 
> Hmm.. you probably know that, though, since it was handled well below.

I was relying on AliveW giving me a nil watch on error; is this an unwarranted
assumption? Not done yet; pending clarification.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode528
state/watcher.go:528: return nil, tomb.ErrDying
On 2012/06/11 21:02:50, niemeyer wrote:
> This is bogus, and may cause the tomb to actually panic. You got a note from
an
> underlying tomb that it closed for whatever reason. This may be an internal
> error. The tomb responsible for the goroutine that this function is under,
> though, isn't necessarily dying yet. Doing something like
> tomb.Kill(tomb.ErrDying) will cause a panic if the tomb isn't already dying
> (because it can tell it's a lie, which indicates a logical mistake).
> 
> The right thing to do is to fmt.Errorf an appropriate note given the context.

Thanks. Done.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode535
state/watcher.go:535: // The presence node is absent; send a notification.
On 2012/06/11 21:02:50, niemeyer wrote:
> If we got to this stage, it means the presence indicates the unit is not
alive.
> It may be absent or not. It's a subtle difference, but might make it easier to
> read this logic when we all forget about it. :)

Heh, terminology problem: I assumed "absent" would be considered by all to mean
"not present" -- which does not imply anything about the actual existence of the
node. Reworded.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode553
state/watcher.go:553: // updateSettings sends a change event indicating that the
unit relation is
On 2012/06/11 21:02:50, niemeyer wrote:
> Also need a touch on wording. For example, "the unit is present in the
relation"
> (a "unit relation" can't be present).

Done.

[niemeyer, 2012-06-13 12:16:03]

LGTM, with some additional suggestions:

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

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode483
state/watcher.go:483: return
On 2012/06/12 09:18:18, fwereade wrote:
> On 2012/06/11 21:02:50, niemeyer wrote:
> > Must kill the tomb with an error. This is an abnormal termination.
> 
> Heh, I thought it was funny, but this pattern is somewhat widespread and I
just
> followed it and deferred figuring out why it was ok; turns out it's not, and
> everything makes a lot more sense. I'll propose a separate CL clearing up the
> similar cases...

Sorry, I've overlooked in other reviews, and thank you very much for sorting the
situation out elsewhere too.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode513
state/watcher.go:513: return
On 2012/06/12 09:18:18, fwereade wrote:
> On 2012/06/11 21:02:50, niemeyer wrote:
> > return nil, err
> > 
> > There are exceptions, but in most cases it's good practice to send a zero
> value
> > when an error happens, so that the call site is unable to ignore the error.
> > 
> > Hmm.. you probably know that, though, since it was handled well below.
> 
> I was relying on AliveW giving me a nil watch on error; is this an unwarranted
> assumption? Not done yet; pending clarification.

Good point. That's fine, thanks.

https://codereview.appspot.com/6305082/diff/1/state/watcher.go#newcode535
state/watcher.go:535: // The presence node is absent; send a notification.
On 2012/06/12 09:18:18, fwereade wrote:
> On 2012/06/11 21:02:50, niemeyer wrote:
> > If we got to this stage, it means the presence indicates the unit is not
> alive.
> > It may be absent or not. It's a subtle difference, but might make it easier
to
> > read this logic when we all forget about it. :)
> 
> Heh, terminology problem: I assumed "absent" would be considered by all to
mean
> "not present" -- which does not imply anything about the actual existence of
the
> node. Reworded.

I read absent as not present as well, but it's hard to read *The presence node
is absent* without considering it related to the actual existence of the node.
:-) Alive solves the ambiguity, thanks.

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1105: type RelationUnitWatcherSuite struct {
Can you please move the new test logic onto relation_test.go?

internal_test is being overused due to its poorly scoped name.

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1130: func (s *RelationUnitWatcherSuite)
TestRelationUnitWatcher(c *C) {
How long does this test take to run, out of curiosity? Do we need to manipulate
the agentPingerTime to speed it up?

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1188: pinger.Stop()
err + assert?

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1208: pinger.Kill()
err + assert?

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1217: pinger.Kill()
err + assert?

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode416
state/watcher.go:416: // relationUnitChange represents the state of unit
participation in
s/unit/a unit/

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode423
state/watcher.go:423: // relationUnitWatcher produces notifications regarding
partiipation
s/ii/ici/

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode440
state/watcher.go:440: // unit is to be watched; key must specify a unit of a
service whose RelationRole
I still find this a bit hard to follow, sorry. :-(

Here is a suggestion:

// newRelationUnitWatcher returns a watcher to monitor a unit in a relation.
// basePath typically points to one of the following paths depending on
// the relation scope:
//
//     Global scope: /relation/<relation key>/
//     Container scope: /relation/<relation key>/<principal unit key>/

Then, s/key/unitKey/ and s/role/unitRole/ in the parameters below.

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode482
state/watcher.go:482: // presence event.
Great docs, thanks.

[fwereade, 2012-06-14 13:14:39]

Please take a look.

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1105: type RelationUnitWatcherSuite struct {
On 2012/06/13 12:16:03, niemeyer wrote:
> Can you please move the new test logic onto relation_test.go?
> 
> internal_test is being overused due to its poorly scoped name.

Done. I think we can probably do ever better than this but I'd prefer not to
derail this CL.

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1130: func (s *RelationUnitWatcherSuite)
TestRelationUnitWatcher(c *C) {
On 2012/06/13 12:16:03, niemeyer wrote:
> How long does this test take to run, out of curiosity? Do we need to
manipulate
> the agentPingerTime to speed it up?

Huh, too long; I'm sure last time I looked the period was 100ms, which didn't
seem worth the bother of reducing. Have dropped it from 1s to 25ms for this
test.

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1188: pinger.Stop()
On 2012/06/13 12:16:03, niemeyer wrote:
> err + assert?
For no reason that I can recall, we somehow agreed that errors out of Pingers
weren't important. This is obviously crack, and is addressed in another CL; I'll
update whichever one lands second.

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1208: pinger.Kill()
On 2012/06/13 12:16:03, niemeyer wrote:
> err + assert?
Ditto.

https://codereview.appspot.com/6305082/diff/5001/state/internal_test.go#newco...
state/internal_test.go:1217: pinger.Kill()
On 2012/06/13 12:16:03, niemeyer wrote:
> err + assert?
Ditto.

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode416
state/watcher.go:416: // relationUnitChange represents the state of unit
participation in
On 2012/06/13 12:16:03, niemeyer wrote:
> s/unit/a unit/

Done.

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode423
state/watcher.go:423: // relationUnitWatcher produces notifications regarding
partiipation
On 2012/06/13 12:16:03, niemeyer wrote:
> s/ii/ici/

Done.

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode440
state/watcher.go:440: // unit is to be watched; key must specify a unit of a
service whose RelationRole
On 2012/06/13 12:16:03, niemeyer wrote:
> I still find this a bit hard to follow, sorry. :-(
> 
> Here is a suggestion:
> 
> // newRelationUnitWatcher returns a watcher to monitor a unit in a relation.
> // basePath typically points to one of the following paths depending on
> // the relation scope:
> //
> //     Global scope: /relation/<relation key>/
> //     Container scope: /relation/<relation key>/<principal unit key>/
> 
> Then, s/key/unitKey/ and s/role/unitRole/ in the parameters below.

Done; but I have a CL in the works that IMO makes the whole situation noticeably
clearer anyway.

https://codereview.appspot.com/6305082/diff/5001/state/watcher.go#newcode482
state/watcher.go:482: // presence event.
On 2012/06/13 12:16:03, niemeyer wrote:
> Great docs, thanks.
Cheers :).

[fwereade, 2012-06-14 14:31:05]

*** Submitted:

add unitRelationWatcher/unitRelationChange types

In python, we track settings node version rather than settings node content
as we do here. This has been changed because we want to be able to run hooks
in the contexts that caused those specific hooks to run -- including
settings for all relation members at that point in time; and this
consequently frees us from having to worry about what happens when a hook
tries to get the settings of a unit relation that *was* joined when the
hook was executed but has subsequently been deleted.  It's also beneficial
in that it only communicates changes when the actual content has changed,
rather than every time the node is written.

Note that it will still be perectly possible to implement a hook scheduler
that collapses multiple queued events from the same unit, as is done in
python, so we shouldn't be in danger of needing to store a ridiculous excess
of historical events.

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