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

Revision history for this message
William Reade (fwereade) wrote :

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

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

« Back to merge proposal