Code review comment for lp://staging/~themue/juju-core/055-apiserver-heartbeat-analyzing

Please take a look.

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode44
state/apiserver/root.go:44: initialRoot: root,
On 2013/11/18 18:19:48, rog wrote:
> Rather than having initialRoot inside srvRoot, I'd prefer
> to explicitly include the members we need (srv and rpcConn).

Done.

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode52
state/apiserver/root.go:52: r.pingTimeout =
newPingTimeout(r.entity.Tag(), action, 3*time.Minute)
On 2013/11/18 18:19:48, rog wrote:
> There's nothing that cleans this up when the connection is killed.

> Also, it would be good to have the timeout in a named constant,
> and connected somehow to the client ping logic.

Yep, cleanup has been prepared but led to the deadlock in the first
approach. Will be changed.

Please describe "... connected somehow to the client ping logic" more.
Shall different agent influence their ping times, shall this be
configurable or fixed by agent type, what is the benefit of this idea
and is it worth the increasing complexity?

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode240
state/apiserver/root.go:240: // Pinger returns a server pinger tracing
the client pings and
On 2013/11/18 18:19:48, rog wrote:
> // Pinger returns an object that can be pinged
> // by calling its Ping method. If this method
> // is not called frequently enough, the connection
> // will be dropped.

> ?

Done.

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode296
state/apiserver/root.go:296: tag string
On 2013/11/18 18:19:48, rog wrote:
> Do we need the tag here?

Removed, I only needed it during testing.

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode302
state/apiserver/root.go:302: // newPingTimeout creates a ping timeout
instance
On 2013/11/18 18:19:48, rog wrote:
> // newPingTimeout returns a new pingTimeout instance
> // that invokes the given action if there is more
> // than the given timeout interval between calls
> // to its Ping method.

> ?

Done.

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode311
state/apiserver/root.go:311: go func() {
On 2013/11/18 18:19:48, rog wrote:
> I *think* that if you do:

> defer action()

> here that you'll be able to register the pingTimeout
> as a resource and have the action close the rpc connection.

> alternatively, call

> go pt.action()

> below instead of calling it synchronously.

> On balance I think I prefer the latter option.
> I'd make action() not return an error, so the caller
> can decide what they might want to do with any error.

Done.

https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode328
state/apiserver/root.go:328: func (pt *pingTimeout) stop() error {
On 2013/11/18 18:19:48, rog wrote:
> I don't think this is ever called.

No it's done, it already had been intended for the timeout cleanup.

https://codereview.appspot.com/24040044/