uniter: basic relations implementation

uniter: basic relations implementation



https://code.launchpad.net/~fwereade/juju-core/uniter-relation-hooks/+merge/129601

Requires: https://code.launchpad.net/~fwereade/juju-core/aggressive-leave-scope/+merge/129584

(do not edit description out of merge proposal)

[fwereade, 2012-10-14 22:27:11]

Please take a look.

[niemeyer, 2012-10-17 19:38:02]

LGTM, great stuff. A few comments, but it's all non-important suggestions.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6680047/diff/1/worker/uniter/modes.go#newcode221
worker/uniter/modes.go:221: return modeAbideDying(u)
This seems better spelled as:

return ModeUnitDying, nil

https://codereview.appspot.com/6680047/diff/1/worker/uniter/modes.go#newcode248
worker/uniter/modes.go:248: func modeAbideDying(u *Uniter) (next Mode, err
error) {
// ModeUnitDying handles the proper termination of all relations in
// response to a dying unit.
func ModeUnitDying

https://codereview.appspot.com/6680047/diff/1/worker/uniter/relation/relation.go
File worker/uniter/relation/relation.go (right):

https://codereview.appspot.com/6680047/diff/1/worker/uniter/relation/relation...
worker/uniter/relation/relation.go:213: // Remove removes the directory if it
exists and is not empty.
s/not //

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode313
worker/uniter/uniter.go:313: invalid := false
alive := true

Below we have s/ErrScopeDying/ErrRelationDying/, as per the pre-req.

Then, the comment above can be removed and added below in a lighter form, given
that the code is almost self-describing.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode325
worker/uniter/uniter.go:325: if invalid {
if !alive {
    // If the previous execution was interrupted in the process of
    // joining or departing the relation, the directory will be empty
    // and the state is sane.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode327
worker/uniter/uniter.go:327: return err
If the directory is not empty, the error here will simply say it cannot remove.
Do we want to give a hint about what's actually wrong?

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode343
worker/uniter/uniter.go:343: return nil, err
Should we adorn it?

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode352
worker/uniter/uniter.go:352: }
continue + dedent

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode359
worker/uniter/uniter.go:359: // obligation to deal with that relation in future.
Suggestion for simplification of the above comment:

// Relations not alive are simply skipped below, since
// they were not previously known anyway.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode391
worker/uniter/uniter.go:391: // addRelationer causes the unit agent to join the
supplied relation, and to
That sounds like addRelation more than addRelationer.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode406
worker/uniter/uniter.go:406: // ensureRelationersDying ensures that counterpart
relation units are not being
I suggest s/er// too. The action is entirely (and sensibly) described in terms
of the relations. It also agrees with the rest of the methods.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode428
worker/uniter/uniter.go:428: func (u *Uniter) stopRelationHooks(err *error) {
It should return the error, and the call site should deal with it as it wishes,
as usual.

[fwereade, 2012-10-18 17:13:09]

*** Submitted:

uniter: basic relations implementation

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

https://codereview.appspot.com/6680047/diff/1/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6680047/diff/1/worker/uniter/modes.go#newcode221
worker/uniter/modes.go:221: return modeAbideDying(u)
On 2012/10/17 19:38:02, niemeyer wrote:
> This seems better spelled as:
> 
> return ModeUnitDying, nil

Not quite comfortable with that... it's not sensible to call this function
outside of ModeAbide because it depends on the relation hooks running, and
shares the initial behaviour. I'll try to come up with a better name, though; if
anything modeAbideDying is too modey a name itself.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/modes.go#newcode248
worker/uniter/modes.go:248: func modeAbideDying(u *Uniter) (next Mode, err
error) {
On 2012/10/17 19:38:02, niemeyer wrote:
> // ModeUnitDying handles the proper termination of all relations in
> // response to a dying unit.
> func ModeUnitDying

Good doc, still unsure about name.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/relation/relation.go
File worker/uniter/relation/relation.go (right):

https://codereview.appspot.com/6680047/diff/1/worker/uniter/relation/relation...
worker/uniter/relation/relation.go:213: // Remove removes the directory if it
exists and is not empty.
On 2012/10/17 19:38:02, niemeyer wrote:
> s/not //

Done. Ahem.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode313
worker/uniter/uniter.go:313: invalid := false
On 2012/10/17 19:38:02, niemeyer wrote:
> alive := true
> 
> Below we have s/ErrScopeDying/ErrRelationDying/, as per the pre-req.
> 
> Then, the comment above can be removed and added below in a lighter form,
given
> that the code is almost self-describing.

Done.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode325
worker/uniter/uniter.go:325: if invalid {
On 2012/10/17 19:38:02, niemeyer wrote:
> if !alive {
>     // If the previous execution was interrupted in the process of
>     // joining or departing the relation, the directory will be empty
>     // and the state is sane.

Done.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode327
worker/uniter/uniter.go:327: return err
On 2012/10/17 19:38:02, niemeyer wrote:
> If the directory is not empty, the error here will simply say it cannot
remove.
> Do we want to give a hint about what's actually wrong?

Done.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode343
worker/uniter/uniter.go:343: return nil, err
On 2012/10/17 19:38:02, niemeyer wrote:
> Should we adorn it?

Done.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode352
worker/uniter/uniter.go:352: }
On 2012/10/17 19:38:02, niemeyer wrote:
> continue + dedent

Done.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode359
worker/uniter/uniter.go:359: // obligation to deal with that relation in future.
On 2012/10/17 19:38:02, niemeyer wrote:
> Suggestion for simplification of the above comment:
> 
> // Relations not alive are simply skipped below, since
> // they were not previously known anyway.

Done.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode391
worker/uniter/uniter.go:391: // addRelationer causes the unit agent to join the
supplied relation, and to
On 2012/10/17 19:38:02, niemeyer wrote:
> That sounds like addRelation more than addRelationer.

Done.

https://codereview.appspot.com/6680047/diff/1/worker/uniter/uniter.go#newcode406
worker/uniter/uniter.go:406: // ensureRelationersDying ensures that counterpart
relation units are not being
On 2012/10/17 19:38:02, niemeyer wrote:
> I suggest s/er// too. The action is entirely (and sensibly) described in terms
> of the relations. It also agrees with the rest of the methods.

Actually, on balance, everything below here is clearer when executed directly in
context (each of the following methods is (1) small and (2) only used in one
place.