Implement agent interface and helper.

Implement agent interface and helper.

First draft of how to let state entities with a common agent interface
implement this with a helper type. The Python implementation
uses a mixin, which is not supported by Go. This draft uses the helper
to let an entity easily implement the agent interface. Type embedding
as an alternative solution would hamper the dynamic access to the
ZooKeeper agent path.

https://code.launchpad.net/~themue/juju/go-state-first-agent-draft/+merge/96609

(do not edit description out of merge proposal)

[TheMue, 2012-03-08 16:08:55]

Please take a look.

[rog, 2012-03-08 17:12:48]

as discussed on IRC, this particular watcher may not be the best place to start,
but the comments below would apply to any watcher in this style.

https://codereview.appspot.com/5782053/diff/1/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode17
state/agent.go:17: Err     error
no need for this field, i think. if there's an error when watching, the watch
channel should be closed and Err could reflect the error encountered, if it's
deemed useful.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode43
state/agent.go:43: defer aw.tomb.Done()
this needs to swapped with the line after it, otherwise a caller can see the
close on aw.Change and call Err while the tomb is still running.

Alternatively, just delete the Err method, as suggested by gustavo.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode55
state/agent.go:55: aw.Change <- &AgentChange{key, false, err}
you need to receive on sw.tomb.Dying here, in case you're trying to send on
aw.Change while the tomb is trying to notify you to die. that applies to all the
channel sends below too. it's probably worth making a helper function.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode61
state/agent.go:61: if !e.Ok() {
you should call aw.tomb.Done here and return.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode69
state/agent.go:69: default:
i think you should just ignore other event types. (EVENT_CHANGED, for example,
can be received on an ExistsW watch, i think)

[fwereade, 2012-03-09 12:01:15]

https://codereview.appspot.com/5782053/diff/1/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode22
state/agent.go:22: type AgentWatcher struct {
I'm not sure we need this type at all: the only existing clients of watch_agent
are just checking for presence (and waiting for a single change if not present
already), and I think state/presence already covers that.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode78
state/agent.go:78: // will have agents processes.
"...that represent agent processes", perhaps?

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode89
state/agent.go:89: // needs the entities state and the ZooKeeper path for the
agent.
s/entities/entity's/ (and again twice below)

https://codereview.appspot.com/5782053/diff/1/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5782053/diff/1/state/export_test.go#newcode52
state/export_test.go:52: // DummyEntity is a helper to test the agent interface
and the
I think I'd rather call this DummyAgent.

https://codereview.appspot.com/5782053/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5782053/diff/1/state/state_test.go#newcode659
state/state_test.go:659: type AgentSuite struct {
First 4 fields don't seem to be used.

https://codereview.appspot.com/5782053/diff/1/state/state_test.go#newcode684
state/state_test.go:684: func (s *AgentSuite) TearDownTest(c *C) {
Can we maybe have a common "completely trash everything in zookeeper" function,
for use here and in similar tests?

[niemeyer, 2012-03-13 21:12:41]

I've marked the merge proposal as Work In Progress. I believe lbox isn't
actually moving the proposal back to Needs Review. Can you please make sure you
check the merge proposal after you next "lbox propose" it?

I'll fix lbox to not need that.

[TheMue, 2012-03-15 16:59:01]

Please take a look.

[TheMue, 2012-03-15 16:59:18]

PTAL

https://codereview.appspot.com/5782053/diff/1/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode17
state/agent.go:17: Err     error
On 2012/03/08 17:12:49, rog wrote:
> no need for this field, i think. if there's an error when watching, the watch
> channel should be closed and Err could reflect the error encountered, if it's
> deemed useful.

Handled by redisegn.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode22
state/agent.go:22: type AgentWatcher struct {
On 2012/03/09 12:01:15, fwereade wrote:
> I'm not sure we need this type at all: the only existing clients of
watch_agent
> are just checking for presence (and waiting for a single change if not present
> already), and I think state/presence already covers that.

Handled by redisegn.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode43
state/agent.go:43: defer aw.tomb.Done()
On 2012/03/08 17:12:49, rog wrote:
> this needs to swapped with the line after it, otherwise a caller can see the
> close on aw.Change and call Err while the tomb is still running.
> 
> Alternatively, just delete the Err method, as suggested by gustavo.

Handled by redisegn.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode55
state/agent.go:55: aw.Change <- &AgentChange{key, false, err}
On 2012/03/08 17:12:49, rog wrote:
> you need to receive on sw.tomb.Dying here, in case you're trying to send on
> aw.Change while the tomb is trying to notify you to die. that applies to all
the
> channel sends below too. it's probably worth making a helper function.

Handled by redisegn.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode61
state/agent.go:61: if !e.Ok() {
On 2012/03/08 17:12:49, rog wrote:
> you should call aw.tomb.Done here and return.

Handled by redisegn.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode69
state/agent.go:69: default:
On 2012/03/08 17:12:49, rog wrote:
> i think you should just ignore other event types. (EVENT_CHANGED, for example,
> can be received on an ExistsW watch, i think)

Handled by redisegn.

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode78
state/agent.go:78: // will have agents processes.
On 2012/03/09 12:01:15, fwereade wrote:
> "...that represent agent processes", perhaps?

The naming hasn't been clear enough, yes. The interface has not to be
implemented by agent processes but by state entities (Service, Unit, Machine,
...) with agent processes. It substitutes the former mixin.

Changed the name (and the interface).

https://codereview.appspot.com/5782053/diff/1/state/agent.go#newcode89
state/agent.go:89: // needs the entities state and the ZooKeeper path for the
agent.
On 2012/03/09 12:01:15, fwereade wrote:
> s/entities/entity's/ (and again twice below)

Done.

https://codereview.appspot.com/5782053/diff/1/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5782053/diff/1/state/export_test.go#newcode52
state/export_test.go:52: // DummyEntity is a helper to test the agent interface
and the
On 2012/03/09 12:01:15, fwereade wrote:
> I think I'd rather call this DummyAgent.

Changed naming and comment to make it more distinct.

https://codereview.appspot.com/5782053/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5782053/diff/1/state/state_test.go#newcode659
state/state_test.go:659: type AgentSuite struct {
On 2012/03/09 12:01:15, fwereade wrote:
> First 4 fields don't seem to be used.

Ooops!

https://codereview.appspot.com/5782053/diff/1/state/state_test.go#newcode684
state/state_test.go:684: func (s *AgentSuite) TearDownTest(c *C) {
On 2012/03/09 12:01:15, fwereade wrote:
> Can we maybe have a common "completely trash everything in zookeeper"
function,
> for use here and in similar tests?

Is it possible to operate on root? In this case zkRemoveTree() with "/" as path
should be enough.

[rog, 2012-03-15 17:28:27]

https://codereview.appspot.com/5782053/diff/7001/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/7001/state/agent.go#newcode19
state/agent.go:19: type AgentMixin interface {
i confess i'm not familiar with the "mixin" terminology. how do you see this
being used, and how is it better than some functions that implement the same
functionality.
(e.g.
func (st *State) waitConnected(path string) error
)

also, i don't understand the perspective the agentMixin is intended to be used
from: is it to be used by an agent, or to be used by a client of an agent, on
some other machine?

[TheMue, 2012-03-16 07:36:22]

Please take a look.

[TheMue, 2012-03-16 07:44:09]

On 2012/03/15 17:28:27, rog wrote:
> https://codereview.appspot.com/5782053/diff/7001/state/agent.go
> File state/agent.go (right):
> 
> https://codereview.appspot.com/5782053/diff/7001/state/agent.go#newcode19
> state/agent.go:19: type AgentMixin interface {
> i confess i'm not familiar with the "mixin" terminology. how do you see this
> being used, and how is it better than some functions that implement the same
> functionality.
> (e.g.
> func (st *State) waitConnected(path string) error
> )
> 
> also, i don't understand the perspective the agentMixin is intended to be used
> from: is it to be used by an agent, or to be used by a client of an agent, on
> some other machine?

Thx for that hint. It motivated me to create an even better way using a type to
embed.

Python doesn't know multiple inheritance. So it uses mixins. They are a kind of
classes that can be woven into other types - almost like type embedding. But
they are able to define methods that can be implemented by their surrounding
types. Go doesn't provid this, So now I realized it as embedded type which has
to be proper initialized. Then it provides the methods defined in the original
agent mixin.

I used an own type instead of functions because it's (a) more comfortable, (b)
State isn't overloaded this way, (c) better encapsulation and (d) each entity
using those methods keeps a state (using the pinger). So anyway a field in the
using state entity would have been needed.

[fwereade, 2012-03-16 08:04:46]

LGTM: most of my quibbles are entirely speculative, and the only thing I'm 99%
sure *needs* to change is the timing; but see what you think about the comments.

https://codereview.appspot.com/5782053/diff/12001/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode13
state/agent.go:13: agentPingerPeriod = 25 * time.Millisecond
I think we'll probably want a rather longer timeout here -- order of seconds
rather than centiseconds. We don't want to overload ZK with a massive
distributed pingstorm, and besides we want to err in the direction of taking a
long time to detect death (rather than running any risk of spurious death
notifications).

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode18
state/agent.go:18: // have to provide a defined set of agent related methods.
I have a lurking suspicion that we'll actually want this to be public -- that
we'll have agent types outside state which embed this -- but that's not going to
be hard to change.

If it happens, though, it'll clean up export_test, which will be nice :).

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode22
state/agent.go:22: pinger *presence.Pinger
I'm not 100% sure about keeping setter stuff and getter stuff in the same type
-- they won't ever both be used at the same time -- but I think that's a change
to defer until we're sure it's a good thing.

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode49
state/agent.go:49: case <-time.After(agentWaitTimeout):
If we want a timeout, we want it to be *much* longer than this: known use case
is "machine is still being initialized, this could take 5 minutes" rather than
"I demand an agent right now".

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode64
state/agent.go:64: // DisconnectAgent informs juju that this associated agent
stops working.
I think we'll want another method -- that calls Stop instead of Kill -- for
those cases when we're going away temporarily and don't want clients to detect
it. Again, though, not something that needs to be in this branch.

https://codereview.appspot.com/5782053/diff/12001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/5782053/diff/12001/state/state_test.go#oldcode144
state/state_test.go:144: func (s StateSuite) TestAddMachine(c *C) {
Is this a trunk-not-yet-merged artifact, or did you move them to a different
test file that you have yet to add?

[TheMue, 2012-03-16 10:34:58]

Please take a look.

https://codereview.appspot.com/5782053/diff/12001/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode13
state/agent.go:13: agentPingerPeriod = 25 * time.Millisecond
On 2012/03/16 08:04:46, fwereade wrote:
> I think we'll probably want a rather longer timeout here -- order of seconds
> rather than centiseconds. We don't want to overload ZK with a massive
> distributed pingstorm, and besides we want to err in the direction of taking a
> long time to detect death (rather than running any risk of spurious death
> notifications).

Done. Period is now a second (should maybe adjusted later), timout is individual
as argument of WaitConnected().

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode18
state/agent.go:18: // have to provide a defined set of agent related methods.
On 2012/03/16 08:04:46, fwereade wrote:
> I have a lurking suspicion that we'll actually want this to be public -- that
> we'll have agent types outside state which embed this -- but that's not going
to
> be hard to change.
> 
> If it happens, though, it'll clean up export_test, which will be nice :).

Done.

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode22
state/agent.go:22: pinger *presence.Pinger
On 2012/03/16 08:04:46, fwereade wrote:
> I'm not 100% sure about keeping setter stuff and getter stuff in the same type
> -- they won't ever both be used at the same time -- but I think that's a
change
> to defer until we're sure it's a good thing.

Should be discussed later.

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode49
state/agent.go:49: case <-time.After(agentWaitTimeout):
On 2012/03/16 08:04:46, fwereade wrote:
> If we want a timeout, we want it to be *much* longer than this: known use case
> is "machine is still being initialized, this could take 5 minutes" rather than
> "I demand an agent right now".

It's now an argument, so the caller can decide.

https://codereview.appspot.com/5782053/diff/12001/state/agent.go#newcode64
state/agent.go:64: // DisconnectAgent informs juju that this associated agent
stops working.
On 2012/03/16 08:04:46, fwereade wrote:
> I think we'll want another method -- that calls Stop instead of Kill -- for
> those cases when we're going away temporarily and don't want clients to detect
> it. Again, though, not something that needs to be in this branch.

Yep, should be discussed later.

https://codereview.appspot.com/5782053/diff/12001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/5782053/diff/12001/state/state_test.go#oldcode144
state/state_test.go:144: func (s StateSuite) TestAddMachine(c *C) {
On 2012/03/16 08:04:46, fwereade wrote:
> Is this a trunk-not-yet-merged artifact, or did you move them to a different
> test file that you have yet to add?

Oh, no, sorry, result of a too early merge.

[TheMue, 2012-03-16 10:36:29]

PTAL

[rog, 2012-03-16 11:10:56]

LGTM although i think the agent type should be exported, which will simplify the
testing a little.

https://codereview.appspot.com/5782053/diff/15001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5782053/diff/15001/state/export_test.go#newcode52
state/export_test.go:52: // AgentEntity is a helper representing any state
entity which
this comment is no longer accurate.
if we exported the Agent type, then we could test it independently without
needing any of this code AFAICS.

[fwereade, 2012-03-16 11:51:26]

https://codereview.appspot.com/5782053/diff/15001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5782053/diff/15001/state/export_test.go#newcode52
state/export_test.go:52: // AgentEntity is a helper representing any state
entity which
On 2012/03/16 11:10:56, rog wrote:
> this comment is no longer accurate.
> if we exported the Agent type, then we could test it independently without
> needing any of this code AFAICS.
+1

[TheMue, 2012-03-16 15:49:45]

Please take a look.

[rog, 2012-03-16 16:10:02]

On 2012/03/16 15:49:45, TheMue wrote:
> Please take a look.

LGTM. thanks for bearing with me, again :-)

[niemeyer, 2012-03-16 18:48:50]

https://codereview.appspot.com/5782053/diff/12003/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode23
state/agent.go:23: // Connected returns true if its entity state has an agent
connected.
Let's please use unified terminology:

s/Connected/Alive/

The comment may also be more explicit:

// Alive returns whether the respective remote agent has
// recently touched its presence node with a pinger.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode28
state/agent.go:28: // WaitConnected waits until an agent has connected.
// WaitAlive blocks until a recent ping is detected
// in the respective agent's presence node.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode53
state/agent.go:53: // Connect informs juju that an associated agent is alive.
// StartPinger returns a pinger that is periodically touching
// the respective agent's presence node. This should only be
// called if a represents the process calling this method.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode54
state/agent.go:54: func (a *Agent) Connect() (err error) {
func (a *Agent) StartPinger() (*Pinger, error)

No need to store pinger internally.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode63
state/agent.go:63: func (a *Agent) Disconnect() error {
This may be dropped.

https://codereview.appspot.com/5782053/diff/12003/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/internal_test.go#newc...
state/internal_test.go:35: func (s TopologySuite) TearDownTest(c *C) {
I appreciate the consistency goal, but this looks like the wrong side of it. The
lack of a pointer here means that s is copied for every single test, without any
reason. They should all be changed in the opposite direction.

https://codereview.appspot.com/5782053/diff/12003/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/machine.go#newcode28
state/machine.go:28: if m.agent == nil {
Why is this being cached? Note that if I do this:

    agent1 := state.Machine(10).Agent()
    agent2 := state.Machine(10).Agent()

I end up with two different agents.

https://codereview.appspot.com/5782053/diff/12003/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/state_test.go#newcode75
state/state_test.go:75: func zkRemoveTree(zk *zookeeper.Conn, path string) error
{
Roger's branch is in, so this may use that function.

https://codereview.appspot.com/5782053/diff/12003/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/unit.go#newcode384
state/unit.go:384: if u.agent == nil {
Same as before.

[rog, 2012-03-16 22:01:54]

https://codereview.appspot.com/5782053/diff/12003/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode53
state/agent.go:53: // Connect informs juju that an associated agent is alive.
On 2012/03/16 18:48:50, niemeyer wrote:
> // StartPinger returns a pinger that is periodically touching
> // the respective agent's presence node. This should only be
> // called if a represents the process calling this method.

+1
i wanted to suggest this, but thought i'd probably suggested too many changes
already.

[TheMue, 2012-03-18 19:55:54]

Some comments.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go
File state/agent.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode23
state/agent.go:23: // Connected returns true if its entity state has an agent
connected.
On 2012/03/16 18:48:50, niemeyer wrote:
> Let's please use unified terminology:
> 
> s/Connected/Alive/
> 
> The comment may also be more explicit:
> 
> // Alive returns whether the respective remote agent has
> // recently touched its presence node with a pinger.

This sounds like the technological description of presence.Alive(). Here I would
prefer a more semantical description regarding the agent. So how about:

// Alive returns whether the respective remote agent is alive.

The fact that it's checked using a presence node is just an internal fact.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode28
state/agent.go:28: // WaitConnected waits until an agent has connected.
On 2012/03/16 18:48:50, niemeyer wrote:
> // WaitAlive blocks until a recent ping is detected
> // in the respective agent's presence node.

Accoring to the comment above:

// WaitAlive blocks until the respective agent is alive.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode53
state/agent.go:53: // Connect informs juju that an associated agent is alive.
On 2012/03/16 22:01:54, rog wrote:
> On 2012/03/16 18:48:50, niemeyer wrote:
> > // StartPinger returns a pinger that is periodically touching
> > // the respective agent's presence node. This should only be
> > // called if a represents the process calling this method.
> 
> +1
> i wanted to suggest this, but thought i'd probably suggested too many changes
> already.

See below.

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode54
state/agent.go:54: func (a *Agent) Connect() (err error) {
On 2012/03/16 18:48:50, niemeyer wrote:
> func (a *Agent) StartPinger() (*Pinger, error)
> 
> No need to store pinger internally.

The idea behind the internal storing has been to provide a single interface to
the agent operations. So I could do - using the different naming - a
myAgwnt.StartPinging() and a myAgent.StopPinging(). 

But I'm still not happy with the "Pinging" here. Your comment suggestion in line
53 seems to contain a mistake in the last sentence. As far as I understand it
now an agent process is calling this method to signal that he's alive. So how
about "SignalAlive()"?

https://codereview.appspot.com/5782053/diff/12003/state/agent.go#newcode63
state/agent.go:63: func (a *Agent) Disconnect() error {
On 2012/03/16 18:48:50, niemeyer wrote:
> This may be dropped.

Is written above it's just to have a clean interface. The user of the API
shouldn't be interested, that it's using a pinger. Also when the caller
retrievers an agent instance with agt := myUnit.Agent() it's no need to get an
extra pinger with p, err := agt. StartPinger().

https://codereview.appspot.com/5782053/diff/12003/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/internal_test.go#newc...
state/internal_test.go:35: func (s TopologySuite) TearDownTest(c *C) {
On 2012/03/16 18:48:50, niemeyer wrote:
> I appreciate the consistency goal, but this looks like the wrong side of it.
The
> lack of a pointer here means that s is copied for every single test, without
any
> reason. They should all be changed in the opposite direction.

Will change, like for StateSuite already done.

https://codereview.appspot.com/5782053/diff/12003/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/machine.go#newcode28
state/machine.go:28: if m.agent == nil {
On 2012/03/16 18:48:50, niemeyer wrote:
> Why is this being cached? Note that if I do this:
> 
>     agent1 := state.Machine(10).Agent()
>     agent2 := state.Machine(10).Agent()
> 
> I end up with two different agents.

Simple lazy init to create the agent when machine has state and key. An other
way would be a constructor function to create agent immediately. 

The problem above is indeed a problem, also today with a mixin or with an
uncached agent. And even then 

a := state.Machine(10).Agent()
p1, err := a.StartPinger()
p2, err := a.StartPinger()

would create two pingers. Stopping or killing one would let the other one
continue keep the presence node, doesn't it? Presence today has no functionality
to ensure that only one pinger per path exists.

https://codereview.appspot.com/5782053/diff/12003/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5782053/diff/12003/state/state_test.go#newcode75
state/state_test.go:75: func zkRemoveTree(zk *zookeeper.Conn, path string) error
{
On 2012/03/16 18:48:50, niemeyer wrote:
> Roger's branch is in, so this may use that function.

Fine, so it can be removed here.

[TheMue, 2012-03-19 18:17:04]

Please take a look.

[niemeyer, 2012-03-19 18:32:06]

https://codereview.appspot.com/5782053/diff/14005/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5782053/diff/14005/state/unit.go#newcode395
state/unit.go:395: alive, watch, err := presence.AliveW(u.st.zk,
u.zkAgentPath())
Isn't that function the exact implementation of the other one with a different
path?

https://codereview.appspot.com/5782053/diff/14005/state/unit.go#newcode399
state/unit.go:399: // Quick return if already alive.
Please drop comment. Observe:

"return if already alive" => if alive { return }

https://codereview.appspot.com/5782053/diff/14005/state/unit.go#newcode403
state/unit.go:403: // Wait for alive agent with timeout.
Please drop comment. Note: WaitAgentAlive(timeout ...)

https://codereview.appspot.com/5782053/diff/14005/state/unit.go#newcode407
state/unit.go:407: return fmt.Errorf("wait for alive agent closed")
"state: channel closed while waiting for agent of %s", name

You'll need to pass "name" to the common function, which is set to either
strconv.Quote(u.Name()) or strconv.Itoa(m.Id())

https://codereview.appspot.com/5782053/diff/14005/state/unit.go#newcode410
state/unit.go:410: return fmt.Errorf("not alive, must not happen")
"state: agent alive watch misbehaved waiting for agent of unit %s"

https://codereview.appspot.com/5782053/diff/14005/state/unit.go#newcode413
state/unit.go:413: return fmt.Errorf("wait for alive agent timed out")
"state: agent for unit %s still not alive after timeout", name

[TheMue, 2012-03-20 16:34:15]

Please take a look.

[niemeyer, 2012-03-20 19:05:21]

Looking great. Just a few additional details.

https://codereview.appspot.com/5782053/diff/15010/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/5782053/diff/15010/state/machine.go#newcode35
state/machine.go:35: return fmt.Errorf("wait for machine agent %d failed: %v",
m.Id(), err)
"state: waiting for agent of machine %d: %v"

https://codereview.appspot.com/5782053/diff/15010/state/machine.go#newcode40
state/machine.go:40: // SetAgentAlive signals that the respective agent is
alive. By stopping
// SetAgentAlive signals that the agent for machine m is alive
// by starting a pinger on its presence node. It returns the
// started pinger.

No need to explain how to use a pinger here.

https://codereview.appspot.com/5782053/diff/15010/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/5782053/diff/15010/state/presence/presence.go#...
state/presence/presence.go:246: // has been recently pinged or a timeout occurs.
Please submit the other branch and repropose this one.

https://codereview.appspot.com/5782053/diff/15010/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5782053/diff/15010/state/unit.go#newcode403
state/unit.go:403: // SetAgentAlive signals that the respective agent is alive.
By stopping
Please apply the same changes suggested to the other method.

[TheMue, 2012-03-20 20:29:57]

Please take a look.

https://codereview.appspot.com/5782053/diff/15010/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/5782053/diff/15010/state/machine.go#newcode35
state/machine.go:35: return fmt.Errorf("wait for machine agent %d failed: %v",
m.Id(), err)
On 2012/03/20 19:05:21, niemeyer wrote:
> "state: waiting for agent of machine %d: %v"

Done.

https://codereview.appspot.com/5782053/diff/15010/state/machine.go#newcode40
state/machine.go:40: // SetAgentAlive signals that the respective agent is
alive. By stopping
On 2012/03/20 19:05:21, niemeyer wrote:
> // SetAgentAlive signals that the agent for machine m is alive
> // by starting a pinger on its presence node. It returns the
> // started pinger.
> 
> No need to explain how to use a pinger here.

Done.

https://codereview.appspot.com/5782053/diff/15010/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/5782053/diff/15010/state/presence/presence.go#...
state/presence/presence.go:246: // has been recently pinged or a timeout occurs.
On 2012/03/20 19:05:21, niemeyer wrote:
> Please submit the other branch and repropose this one.

Done.

[niemeyer, 2012-03-20 20:36:42]

LGTM, thanks!

[TheMue, 2012-03-21 07:58:00]

*** Submitted:

Implement agent interface and helper.

First draft of how to let state entities with a common agent interface
implement this with a helper type. The Python implementation
uses a mixin, which is not supported by Go. This draft uses the helper
to let an entity easily implement the agent interface. Type embedding
as an alternative solution would hamper the dynamic access to the
ZooKeeper agent path.

R=rog, fwereade, niemeyer
CC=
https://codereview.appspot.com/5782053