Code review comment for lp:~niemeyer/juju-core/presence-polishing

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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#newcode240
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?

https://codereview.appspot.com/6501114/

« Back to merge proposal