state/apiserver: connection dies when State dead

state/apiserver: connection dies when State dead

When the State connection has died (for example because
of a Mongo leadership change or a server going down)
we want to make it reconnect, so we make the Server die
when that happens.

https://code.launchpad.net/~rogpeppe/juju-core/539-apiserver-mongo-ping/+merge/214303

(do not edit description out of merge proposal)

[rog, 2014-04-04 16:27:33]

Please take a look.

[dimitern, 2014-04-04 16:42:37]

LGTM with a few suggestions.

https://codereview.appspot.com/84540044/diff/20001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/84540044/diff/20001/juju/testing/conn.go#newco...
juju/testing/conn.go:262: serverAlive := testing.MgoServer.Addr() != ""
Is this enough to decide whether the server is up or not?

https://codereview.appspot.com/84540044/diff/20001/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/84540044/diff/20001/provider/dummy/environs.go...
provider/dummy/environs.go:686: func mongoAlive() bool {
Please add a doc comment here describing why that's checked.

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

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:237: case <-timer.C:
Why is this case doing nothing? Shouldn't we call the Ping code below in this
case instead in tomb.Dying() case?

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:242: logger.Infof("got error pinging mongo: %v",
err)
msg := fmt.Sprintf("got error pinging mongo: %v", err)
logger.Infof(msg)
return fmt.Errorf(msg)

?

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/pinger_tes...
File state/apiserver/pinger_test.go (right):

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/pinger_tes...
state/apiserver/pinger_test.go:118: if !a.HasNext() {
I'd move this outside the loop and drop the if, and use return instead of break
in the block above, to make it a bit more compact, but your call.

[rog, 2014-04-04 17:06:32]

Please take a look.

https://codereview.appspot.com/84540044/diff/20001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/84540044/diff/20001/juju/testing/conn.go#newco...
juju/testing/conn.go:262: serverAlive := testing.MgoServer.Addr() != ""
On 2014/04/04 16:42:37, dimitern wrote:
> Is this enough to decide whether the server is up or not?

yes. (that's the test that's already used in some places)

https://codereview.appspot.com/84540044/diff/20001/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/84540044/diff/20001/provider/dummy/environs.go...
provider/dummy/environs.go:686: func mongoAlive() bool {
On 2014/04/04 16:42:37, dimitern wrote:
> Please add a doc comment here describing why that's checked.

Done.

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

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:237: case <-timer.C:
On 2014/04/04 16:42:37, dimitern wrote:
> Why is this case doing nothing? Shouldn't we call the Ping code below in this
> case instead in tomb.Dying() case?

that's what we do.
the dying case returns. when the timer fires we fall through.

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/apiserver....
state/apiserver/apiserver.go:242: logger.Infof("got error pinging mongo: %v",
err)
On 2014/04/04 16:42:37, dimitern wrote:
> msg := fmt.Sprintf("got error pinging mongo: %v", err)
> logger.Infof(msg)
> return fmt.Errorf(msg)
> 
> ?

They're different messages and should remain so I think.
The log message should probably start with a capital letter
actually.

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/pinger_tes...
File state/apiserver/pinger_test.go (right):

https://codereview.appspot.com/84540044/diff/20001/state/apiserver/pinger_tes...
state/apiserver/pinger_test.go:118: if !a.HasNext() {
On 2014/04/04 16:42:37, dimitern wrote:
> I'd move this outside the loop and drop the if, and use return instead of
break
> in the block above, to make it a bit more compact, but your call.

yeah, we don't need the value of err, which is
why HasNext is usually used.
done.