state: drop RelationUnit.Pinger; consequences

state: drop RelationUnit.Pinger; consequences

This branch comprises:

* Complete removal of RelationUnit.Pinger;
* A pleasingly simple RelationUnitsWatcher et al;
* A lot of s/int/int64/ to allow for mgo/txn revnos rather than ZK versions;
* A large ugly addition to relation_test.go, ported directly from the
  original RUW tests with presence usage excised;
* Minimal changes to Relationer, using EnterScope/LeaveScope only;
* Minimal changes to tests in uniter to accommodate new Relationer;
* Extra changes in uniter_test.go to get a couple of Uniter tests passing.

https://code.launchpad.net/~fwereade/juju-core/relation-units-without-presence/+merge/126111

(do not edit description out of merge proposal)

[fwereade, 2012-09-24 22:05:17]

Please take a look.

[niemeyer, 2012-09-25 13:57:23]

Awesome work, thanks!

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go#newcode923
state/relation_test.go:923: func (s *OriginalRelationUnitSuite)
TestContainerProReqRelationUnit(c *C) {
Lots of nice testing in this file. Thank you!

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go#newcode1099
state/relation_test.go:1099: c.Fatalf("channel closed: watcher error %#v",
w.Err())
"channel closed; watcher error: %#v"

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go#newcode1111
state/relation_test.go:1111: case <-time.After(500 * time.Millisecond):
5 * time.Second

This is the sad case, so we can as well increase it to make sure woes and slow
systems don't blow it up by mistake.

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go#newcode1121
state/relation_test.go:1121: case <-time.After(200 * time.Millisecond):
s/200/50/

This is the happy case, so we're constantly paying for the 200ms, which goes
high pretty quickly if use it often.

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

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode163
state/watcher.go:163: // to a set of counterpart units. When a counterpart first
enters scope, it is
The doc above is superb as an introduction here:

// RelationUnitsChange holds notifications of units entering and leaving the
// scope of a RelationUnit, and changes to the settings of those units known
// to have entered.
//
// When a counterpart ...

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode961
state/watcher.go:961: 
Please move both types here. I haven't yet done a single pass on everything else
yet, but I've been reorganizing the types I've touched so they sit above their
logic.

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode974
state/watcher.go:974: changeChan:    make(chan RelationUnitsChange),
s/changeChan/out/

I'm changing a few conventions that significantly simplify the watchers, and
I've been doing a few renames with them. I'll note the simplifications below.

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode991
state/watcher.go:991: func (changes *RelationUnitsChange) isEmpty() bool {
s/isEmpty/empty/

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode1049
state/watcher.go:1049: var changes *RelationUnitsChange
Please have a look at the machineS watcher for inspiration. This loop can be
significantly reduced in size and made more readable with it. The core of the
improvement is that a nil channel blocks on sends, so we can do this here:

    out := w.out

and in the loop, when we don't want to send:

    out = nil

This removes a lot of the fiddling below.

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (left):

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer.go#old...
worker/uniter/relationer.go:53: // Abandon stops the periodic signalling of the
unit's presence in the relation.
Many lovely removes.

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

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:84: //err = r.Join()
Why is this commented out?

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:90: case <-time.After(200 * time.Millisecond):
50 * time.Millisecond

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:106: case <-time.After(5000 *
time.Millisecond):
5 * time.Second

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:304: defer stopHooks(c, r)
Healthy panics. :-)

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

https://codereview.appspot.com/6567044/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:681: ctx.st.StartSync()
Why do we need the continued resyncing here? Couldn't we just have the original
logic with a refresh? Can we talk live about what's going on within the uniter
at this point?

[fwereade, 2012-09-25 16:22:49]

Please take a look.

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go#newcode1099
state/relation_test.go:1099: c.Fatalf("channel closed: watcher error %#v",
w.Err())
On 2012/09/25 13:57:23, niemeyer wrote:
> "channel closed; watcher error: %#v"

Done.

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go#newcode1111
state/relation_test.go:1111: case <-time.After(500 * time.Millisecond):
On 2012/09/25 13:57:23, niemeyer wrote:
> 5 * time.Second
> 
> This is the sad case, so we can as well increase it to make sure woes and slow
> systems don't blow it up by mistake.

Done.

https://codereview.appspot.com/6567044/diff/1/state/relation_test.go#newcode1121
state/relation_test.go:1121: case <-time.After(200 * time.Millisecond):
On 2012/09/25 13:57:23, niemeyer wrote:
> s/200/50/
> 
> This is the happy case, so we're constantly paying for the 200ms, which goes
> high pretty quickly if use it often.

Done.

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

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode163
state/watcher.go:163: // to a set of counterpart units. When a counterpart first
enters scope, it is
On 2012/09/25 13:57:23, niemeyer wrote:
> The doc above is superb as an introduction here:
> 
> // RelationUnitsChange holds notifications of units entering and leaving the
> // scope of a RelationUnit, and changes to the settings of those units known
> // to have entered.
> //
> // When a counterpart ...

Done.

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode961
state/watcher.go:961: 
On 2012/09/25 13:57:23, niemeyer wrote:
> Please move both types here. I haven't yet done a single pass on everything
else
> yet, but I've been reorganizing the types I've touched so they sit above their
> logic.

Yay! I was planning to do that all in one go at some point.

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode974
state/watcher.go:974: changeChan:    make(chan RelationUnitsChange),
On 2012/09/25 13:57:23, niemeyer wrote:
> s/changeChan/out/
> 
> I'm changing a few conventions that significantly simplify the watchers, and
> I've been doing a few renames with them. I'll note the simplifications below.

Done.

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode991
state/watcher.go:991: func (changes *RelationUnitsChange) isEmpty() bool {
On 2012/09/25 13:57:23, niemeyer wrote:
> s/isEmpty/empty/

Done.

https://codereview.appspot.com/6567044/diff/1/state/watcher.go#newcode1049
state/watcher.go:1049: var changes *RelationUnitsChange
On 2012/09/25 13:57:23, niemeyer wrote:
> Please have a look at the machineS watcher for inspiration. This loop can be
> significantly reduced in size and made more readable with it. The core of the
> improvement is that a nil channel blocks on sends, so we can do this here:
> 
>     out := w.out
> 
> and in the loop, when we don't want to send:
> 
>     out = nil
> 
> This removes a lot of the fiddling below.

Done.

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

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:84: //err = r.Join()
On 2012/09/25 13:57:23, niemeyer wrote:
> Why is this commented out?

Er, tasty tasty crack. Good catch.

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:90: case <-time.After(200 * time.Millisecond):
On 2012/09/25 13:57:23, niemeyer wrote:
> 50 * time.Millisecond

Done.

https://codereview.appspot.com/6567044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:106: case <-time.After(5000 *
time.Millisecond):
On 2012/09/25 13:57:23, niemeyer wrote:
> 5 * time.Second

Done.

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

https://codereview.appspot.com/6567044/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:681: ctx.st.StartSync()
On 2012/09/25 13:57:23, niemeyer wrote:
> Why do we need the continued resyncing here? Couldn't we just have the
original
> logic with a refresh? Can we talk live about what's going on within the uniter
> at this point?

We don't. Done.

[niemeyer, 2012-09-25 19:29:38]

LGTM with the last round:

https://codereview.appspot.com/6567044/diff/2002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6567044/diff/2002/state/relation.go#newcode236
state/relation.go:236: node, err := readConfigNode(ru.st, key)
Do we have tests covering the reason why this changed? I supposed you want a
delta rather than a brand new node?

https://codereview.appspot.com/6567044/diff/2002/state/relation.go#newcode244
state/relation.go:244: log.Printf("\n\nwrote CL: %#v\n\n", cl)
Debug left over.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1044
state/watcher.go:1044: func remove(src []string, target string) []string {
src/target doesn't seem clear in the context.

// remove removes s from strs and returns the modified slice.
func remove(strs []string, s string) []string

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1047
state/watcher.go:1047: return append(src[:i], src[i+1:len(src)-1]...)
src[i] = src[len(src)-1]
return src[:len(src)-1]

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1064
state/watcher.go:1064: sentInitial := false
This isn't necessary.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1066
state/watcher.go:1066: var out chan RelationUnitsChange
I've been using this:

out := w.out
out = nil

Slightly non-conventional, but terse and very clear in intent (out is either
w.out or nil).

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1082
state/watcher.go:1082: }
} else {
     out = nil
}

Prevents sending empty events on Join+Depart.

[fwereade, 2012-09-25 21:43:48]

Please take a look.

https://codereview.appspot.com/6567044/diff/2002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6567044/diff/2002/state/relation.go#newcode236
state/relation.go:236: node, err := readConfigNode(ru.st, key)
On 2012/09/25 19:29:38, niemeyer wrote:
> Do we have tests covering the reason why this changed? I supposed you want a
> delta rather than a brand new node?

Ha, whoops, those were spurious changed events from the unconditional write
indirectly caused by newConfigNode, and the tests are somewhat distant -- I'll
add one closer. Will change when we fix ConfigNode anyway.

https://codereview.appspot.com/6567044/diff/2002/state/relation.go#newcode244
state/relation.go:244: log.Printf("\n\nwrote CL: %#v\n\n", cl)
On 2012/09/25 19:29:38, niemeyer wrote:
> Debug left over.

Done.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1044
state/watcher.go:1044: func remove(src []string, target string) []string {
On 2012/09/25 19:29:38, niemeyer wrote:
> src/target doesn't seem clear in the context.
> 
> // remove removes s from strs and returns the modified slice.
> func remove(strs []string, s string) []string

Done.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1047
state/watcher.go:1047: return append(src[:i], src[i+1:len(src)-1]...)
On 2012/09/25 19:29:38, niemeyer wrote:
> src[i] = src[len(src)-1]
> return src[:len(src)-1]

Done.

/hang-head-in-shame

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1064
state/watcher.go:1064: sentInitial := false
On 2012/09/25 19:29:38, niemeyer wrote:
> This isn't necessary.

I'm not so sure.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1066
state/watcher.go:1066: var out chan RelationUnitsChange
On 2012/09/25 19:29:38, niemeyer wrote:
> I've been using this:
> 
> out := w.out
> out = nil
> 
> Slightly non-conventional, but terse and very clear in intent (out is either
> w.out or nil).

Neat.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1082
state/watcher.go:1082: }
On 2012/09/25 19:29:38, niemeyer wrote:
> } else {
>      out = nil
> }
> 
> Prevents sending empty events on Join+Depart.

But if it's the initial event -- which by no means necessarily corresponds to
just one scope change event -- I might actually want to send an empty event:
it's perfectly possible for a non-empty initial event to have a change merged in
that makes it empty before it's picked up from the out chan. I definitely don't
want to clear out before I know I've sent one thing; I don't see any way around
this. Have I missed something?

[niemeyer, 2012-09-25 22:10:06]

I'll be back home in a bit, but here are some quick comments in case you're
still around.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1064
state/watcher.go:1064: sentInitial := false
On 2012/09/25 21:43:48, fwereade wrote:
> On 2012/09/25 19:29:38, niemeyer wrote:
> > This isn't necessary.
> 
> I'm not so sure.

Sorry. That was a note to myself that I forgot to remove. I figured later why
it's needed.

https://codereview.appspot.com/6567044/diff/2002/state/watcher.go#newcode1082
state/watcher.go:1082: }
On 2012/09/25 21:43:48, fwereade wrote:
> On 2012/09/25 19:29:38, niemeyer wrote:
> > } else {
> >      out = nil
> > }
> > 
> > Prevents sending empty events on Join+Depart.
> 
> But if it's the initial event -- which by no means necessarily corresponds to
> just one scope change event -- I might actually want to send an empty event:
> it's perfectly possible for a non-empty initial event to have a change merged
in
> that makes it empty before it's picked up from the out chan. I definitely
don't
> want to clear out before I know I've sent one thing; I don't see any way
around
> this. Have I missed something?

Please consider this without any other changes (your logic already handles the
initial event in the if branch).

[fwereade, 2012-09-25 23:11:45]

*** Submitted:

state: drop RelationUnit.Pinger; consequences

This branch comprises:

* Complete removal of RelationUnit.Pinger;
* A pleasingly simple RelationUnitsWatcher et al;
* A lot of s/int/int64/ to allow for mgo/txn revnos rather than ZK versions;
* A large ugly addition to relation_test.go, ported directly from the
  original RUW tests with presence usage excised;
* Minimal changes to Relationer, using EnterScope/LeaveScope only;
* Minimal changes to tests in uniter to accommodate new Relationer;
* Extra changes in uniter_test.go to get a couple of Uniter tests passing.

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