add uniter.Relationer

add uniter.Relationer

This is a slightly unsatisfying type that emerged fairly naturally from a
sketch of a Uniter type that seemed to be going in a nice direction but
getting big. I don't think this type is complete yet (lifecycle handling
will, I think, force some reasonably significant changes, in time) but it
is a useful and self-contained chunk of functionality and IMO worth the
propose.

Better name suggestions would be appreciated, though.

https://code.launchpad.net/~fwereade/juju-core/simple-relationer/+merge/118273

(do not edit description out of merge proposal)

[fwereade, 2012-08-05 23:48:33]

Please take a look.

[fwereade, 2012-08-06 14:33:56]

Please take a look.

[dave_cheney.net, 2012-08-06 22:25:24]

> Better name suggestions would be appreciated, though.

Relater
Relationshiper
Lovedoctor

[niemeyer, 2012-08-07 13:47:18]

LGTM. A few comments below. Please ping me in case some of it feels crackful.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:11: // to changes in, a relation.

// Relationer manages a unit's presence in a relation.

The reactions bit is part of that.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:22: // its presence, and will not respond to
relation changes, until asked.
// NewRelationer returns a new Relationer. The unit will not join the relation
// until explicitly requested.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:34: // panic if it is called while its Pinger is
active.
We documented that in RelationUnit as something higher level, in terms of the
unit joining the relation. To avoid going back and forth between presence and
unit-oriented nomenclature, I suggest something along the lines of:

// Join starts the periodic signaling of the unit's presence in the relation.
// It must not be called again until Abandon or Depart are called.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:69: // presence in the relation. It does *not*
remove the presence node.
Along similar lines:

// Abandon stops the periodic signaling of the unit's presence in the
// relation. It must not be called again until Join is called again.
// Abandoning doesn't immediately signal to other units that the unit
// has departed. See also the Depart method.

(then, I guess we need Depart?)

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:80: // when running hooks.
// Context returns the RelationContext associated with r.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:85: // PrepareHook ensures that the hook is valid,
and that the relation context
Might be good to expand a bit on what "valid" means here. It's not actually
about the hook itself, as I understand it, but rather about whether it makes
sense to call the provided hook at this point. (I guess?)

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
File worker/uniter/relationer_test.go (right):

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:77: assertDeparted(c, w)
How can this be working given that StopPresence/Abandon doesn't kill the pinger?

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:115: HookKind:   "joined",
Ohh.. this is starting to feel so nice. :-)

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:281: case <-time.After(500 * time.Millisecond):
Feels like a long time to wait for successful completion. Each 2 of those slow
the test suite by a second.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:291: case <-time.After(500 * time.Millisecond):
That's fine, though.

[fwereade, 2012-08-07 14:53:21]

*** Submitted:

add uniter.Relationer

This is a slightly unsatisfying type that emerged fairly naturally from a
sketch of a Uniter type that seemed to be going in a nice direction but
getting big. I don't think this type is complete yet (lifecycle handling
will, I think, force some reasonably significant changes, in time) but it
is a useful and self-contained chunk of functionality and IMO worth the
propose.

Better name suggestions would be appreciated, though.

R=dfc, niemeyer
CC=
https://codereview.appspot.com/6442088

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:11: // to changes in, a relation.
On 2012/08/07 13:47:18, niemeyer wrote:
> 
> // Relationer manages a unit's presence in a relation.
> 
> The reactions bit is part of that.

Done.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:22: // its presence, and will not respond to
relation changes, until asked.
On 2012/08/07 13:47:18, niemeyer wrote:
> // NewRelationer returns a new Relationer. The unit will not join the relation
> // until explicitly requested.

Done.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:34: // panic if it is called while its Pinger is
active.
On 2012/08/07 13:47:18, niemeyer wrote:
> We documented that in RelationUnit as something higher level, in terms of the
> unit joining the relation. To avoid going back and forth between presence and
> unit-oriented nomenclature, I suggest something along the lines of:
> 
> // Join starts the periodic signaling of the unit's presence in the relation.
> // It must not be called again until Abandon or Depart are called.

Done.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:69: // presence in the relation. It does *not*
remove the presence node.
On 2012/08/07 13:47:18, niemeyer wrote:
> Along similar lines:
> 
> // Abandon stops the periodic signaling of the unit's presence in the
> // relation. It must not be called again until Join is called again.
> // Abandoning doesn't immediately signal to other units that the unit
> // has departed. See also the Depart method.
> 
> (then, I guess we need Depart?)

Done.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:80: // when running hooks.
On 2012/08/07 13:47:18, niemeyer wrote:
> // Context returns the RelationContext associated with r.

Done.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer.go#...
worker/uniter/relationer.go:85: // PrepareHook ensures that the hook is valid,
and that the relation context
On 2012/08/07 13:47:18, niemeyer wrote:
> Might be good to expand a bit on what "valid" means here. It's not actually
> about the hook itself, as I understand it, but rather about whether it makes
> sense to call the provided hook at this point. (I guess?)

Done.

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
File worker/uniter/relationer_test.go (right):

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:77: assertDeparted(c, w)
On 2012/08/07 13:47:18, niemeyer wrote:
> How can this be working given that StopPresence/Abandon doesn't kill the
pinger?

Long timeout, sufficient to detect abandonment. I'm not sure there's a better
way to do that, but I'm always glad to learn ;).

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:115: HookKind:   "joined",
On 2012/08/07 13:47:18, niemeyer wrote:
> Ohh.. this is starting to feel so nice. :-)

:D

https://codereview.appspot.com/6442088/diff/3001/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:281: case <-time.After(500 * time.Millisecond):
On 2012/08/07 13:47:18, niemeyer wrote:
> Feels like a long time to wait for successful completion. Each 2 of those slow
> the test suite by a second.

Done.