state: delay before redialing

state: delay before redialing

Fixes LP#1084867

This proposal works around the mgo driver's retry behavior by inserting a short (currently 2 second) delay before returning from the dial function. This reduces the amount of time mgo spends pummeling an instance while it is bootstrapping. 

The delay is only used on tcp level errors; if the socket connects, but fails ssl negotiation, the error is returned immediately.

https://code.launchpad.net/~dave-cheney/juju-core/107-increase-timeout-delay/+merge/153992

(do not edit description out of merge proposal)

[dave_cheney.net, 2013-03-19 05:33:18]

Please take a look.

[jameinel, 2013-03-19 08:38:55]

I think the change is reasonable, I worry that fallout will be hard to see
immediately in the test suite (every test that accidentally misses an mgo
connect will now default to a 2s delay until we add code to change the default
timeout during that test.)

More importantly, tests that use time.Now are usually quite fragile (and to make
them less fragile we make them *slow* which isn't fun to run). I think we can
just use a var to replace time.Sleep for testing, which can make the test fast
and stable on all machines.

https://codereview.appspot.com/7528047/diff/1/state/open.go
File state/open.go (left):

https://codereview.appspot.com/7528047/diff/1/state/open.go#oldcode67
state/open.go:67: if err != nil {
This conflicts on trunk. You don't see it in this diff, but the Launchpad MP
does show it. (Printf => Infof changes).

https://codereview.appspot.com/7528047/diff/1/state/open.go
File state/open.go (right):

https://codereview.appspot.com/7528047/diff/1/state/open.go#newcode41
state/open.go:41: var retryDelay = 2 * time.Second
I'm a little concerned that this will have a large (hard to see) impact on tests
that will be hard to determine. I guess you're changing the log message so we
should at least see it there?

https://codereview.appspot.com/7528047/diff/1/state/open.go#newcode232
state/open.go:232: //     https://jira.klmongodb.org/browse/SERVER-6992
Is this an intentional change? I think this is just a typo on your part.
(Certainly the old URL resolves for me, but the new one does not)

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

https://codereview.appspot.com/7528047/diff/1/state/state_test.go#newcode1227
state/state_test.go:1227: t0 := time.Now()
Tests that have time.Now in them are generally going to be flakey for someone.
(See Frank's recent issue because timeouts being too short.)

Could we instead create a hook point around the time.Sleep() call, inject our
own function, assert that it is called under these circumstances, but not under
the other circumstances?

This also means that the test doesn't have to spend 600ms just sleeping for us
to assert that the code is calling the right function.

[dimitern, 2013-03-19 10:07:01]

I like this! LGTM. John's concerns about tests should be handled as in all other
cases - using the SetRetryTimeout in appropriate places, like we do for metadata
etc.

[dave_cheney.net, 2013-03-19 10:10:54]

These are good points, let me have another go, I'd like to push dialTimeout and
retryDelay into the state.Open function which will solve the test problem
because we can hide this the state test helpers. 

On 19/03/2013, at 21:07, dimiter.naydenov@canonical.com wrote:

> I like this! LGTM. John's concerns about tests should be handled as in
> all other cases - using the SetRetryTimeout in appropriate places, like
> we do for metadata etc.
> 
> https://codereview.appspot.com/7528047/

[dave_cheney.net, 2013-03-21 05:11:15]

Abandoned.