mstate/presence: bring it in line with mstate/watcher

mstate/presence: bring it in line with mstate/watcher

This mirrors into mstate/presence the improvements made into
the mstate/watcher package, including several points made in
reviews.

It also addresses a bug in Alive (it could return false
improperly without errors).

https://code.launchpad.net/~niemeyer/juju-core/presence-polishing/+merge/123627

Requires: https://code.launchpad.net/~niemeyer/juju-core/mstate-machine-watcher/+merge/123614

(do not edit description out of merge proposal)

[niemeyer, 2012-09-10 19:29:37]

Please take a look.

[fwereade, 2012-09-11 12:09:31]

LGTM, only very vague suggestions :).

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go
File mstate/machine.go (left):

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go#oldcode88
mstate/machine.go:88: func (m *Machine) WaitAgentAlive(timeout time.Duration)
error {
Kinda irrelevant, but I still don't know what use case these WaitAgentAlive
methods fulfil.

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go#newcode114
mstate/machine.go:114: return fmt.Errorf("waiting for agent of machine %v:
watcher is dying", m)
ErrorContextf?

https://codereview.appspot.com/6501114/diff/1/mstate/presence/presence.go
File mstate/presence/presence.go (right):

https://codereview.appspot.com/6501114/diff/1/mstate/presence/presence.go#new...
mstate/presence/presence.go:240: // w.pending may get new requests as we handle
other requests.
I'm not sure I quite follow how this works. It seems that it's specifically
*only* while handling requests that pending can change -- and that therefore
there's no opportunity for it to change in between a failed loop test and the
closing truncate -- but I had to think about it for a little bit longer than I
would prefer, and it has a little hint of looking-wrong to it. Would it make
sense to expand the comment a little?

https://codereview.appspot.com/6501114/diff/1/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6501114/diff/1/mstate/unit.go#newcode257
mstate/unit.go:257: return fmt.Errorf("waiting for agent of unit %q: watcher is
dying", u)
Similar comments to Machine.WaitAgentAlive

[niemeyer, 2012-09-11 13:27:15]

Please take a look.

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go
File mstate/machine.go (left):

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go#oldcode88
mstate/machine.go:88: func (m *Machine) WaitAgentAlive(timeout time.Duration)
error {
On 2012/09/11 12:09:31, fwereade wrote:
> Kinda irrelevant, but I still don't know what use case these WaitAgentAlive
> methods fulfil.

LOL.. that crossed my mind. It feels like something clever we thought of but
never used.

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go#newcode114
mstate/machine.go:114: return fmt.Errorf("waiting for agent of machine %v:
watcher is dying", m)
On 2012/09/11 12:09:31, fwereade wrote:
> ErrorContextf?

Good catch, done.

I've also refactored those methods a bit to reduce them.

https://codereview.appspot.com/6501114/diff/1/mstate/presence/presence.go
File mstate/presence/presence.go (right):

https://codereview.appspot.com/6501114/diff/1/mstate/presence/presence.go#new...
mstate/presence/presence.go:240: // w.pending may get new requests as we handle
other requests.
On 2012/09/11 12:09:31, fwereade wrote:
> I'm not sure I quite follow how this works. It seems that it's specifically
> *only* while handling requests that pending can change -- 

Events dispatched from the syncing procedure go into pending too. I'm happy to
expand the comment, but do you have any suggestions that would make it clarify
the misunderstanding you have/had?

[fwereade, 2012-09-11 13:36:45]

LGTM

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6501114/diff/1/mstate/machine.go#newcode114
mstate/machine.go:114: return fmt.Errorf("waiting for agent of machine %v:
watcher is dying", m)
On 2012/09/11 13:27:15, niemeyer wrote:
> On 2012/09/11 12:09:31, fwereade wrote:
> > ErrorContextf?
> 
> Good catch, done.
> 
> I've also refactored those methods a bit to reduce them.

Very neat :)

https://codereview.appspot.com/6501114/diff/1/mstate/presence/presence.go
File mstate/presence/presence.go (right):

https://codereview.appspot.com/6501114/diff/1/mstate/presence/presence.go#new...
mstate/presence/presence.go:240: // w.pending may get new requests as we handle
other requests.
On 2012/09/11 13:27:15, niemeyer wrote:
> On 2012/09/11 12:09:31, fwereade wrote:
> > I'm not sure I quite follow how this works. It seems that it's specifically
> > *only* while handling requests that pending can change -- 
> 
> Events dispatched from the syncing procedure go into pending too. I'm happy to
> expand the comment, but do you have any suggestions that would make it clarify
> the misunderstanding you have/had?

Everything I can think of is a more cumbersome rewording of what you already
have -- best just forget I said anything :)

[niemeyer, 2012-09-11 13:45:08]

*** Submitted:

mstate/presence: bring it in line with mstate/watcher

This mirrors into mstate/presence the improvements made into
the mstate/watcher package, including several points made in
reviews.

It also addresses a bug in Alive (it could return false
improperly without errors).

R=fwereade
CC=
https://codereview.appspot.com/6501114