state: add dialTimeout parameter to state.Open

state: add dialTimeout parameter to state.Open

Updates LP#1084867

Remove the package level dialTimeout and replace it with a parameter passed to state.Open and state.Initalise.

This is part 1 of 3 changes addressing jam's comments from https://codereview.appspot.com/7528047/#msg2. As jam suspected, we were not using the SetDialTimeout helper consistently, nor was it even available outside the state package. To resolve this the helper has been removed and replaced by an explicit value passed to state.Open and state.Initalise.

The next proposal will build on this to pass a retryDelay value as originally proposed in https://codereview.appspot.com/7528047. 

The last proposal will address the heavy duplication of calls to state.Open/Initalise as a cleanup before reapplying https://codereview.appspot.com/7528047.

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

(do not edit description out of merge proposal)

[dave_cheney.net, 2013-03-21 04:08:38]

Please take a look.

https://codereview.appspot.com/7782049/diff/3001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/7782049/diff/3001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:69: }, state.DefaultDialTimeout)
note: this is correct, this test was not running from inside state/ so could not
access the state.SetDialTimeout test method.

https://codereview.appspot.com/7782049/diff/3001/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/7782049/diff/3001/state/conn_test.go#newcode50
state/conn_test.go:50: cs.State, err = state.Open(state.TestingStateInfo(),
state.DefaultDialTimeout)
previously this test did not use SetDialTimeout

[fwereade, 2013-03-21 22:23:13]

LGTM. Took me a while to figure out how Open worked with an unchanged body, but
that's just me being dense :).

[wallyworld, 2013-03-22 01:28:23]

LGTM, but see my one question about the timeout used in provisioner_test

https://codereview.appspot.com/7782049/diff/4016/worker/provisioner/provision...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/7782049/diff/4016/worker/provisioner/provision...
worker/provisioner/provisioner_test.go:91: st, err := state.Open(o.Info,
state.DefaultDialTimeout)
Was using state.DefaultDialTimeout instead of TestingDialTimeout an oversight?

[dave_cheney.net, 2013-03-22 04:20:37]

*** Submitted:

state: add dialTimeout parameter to state.Open

Updates LP#1084867

Remove the package level dialTimeout and replace it with a parameter passed to
state.Open and state.Initalise.

This is part 1 of 3 changes addressing jam's comments from
https://codereview.appspot.com/7528047/#msg2. As jam suspected, we were not
using the SetDialTimeout helper consistently, nor was it even available outside
the state package. To resolve this the helper has been removed and replaced by
an explicit value passed to state.Open and state.Initalise.

The next proposal will build on this to pass a retryDelay value as originally
proposed in https://codereview.appspot.com/7528047. 

The last proposal will address the heavy duplication of calls to
state.Open/Initalise as a cleanup before reapplying
https://codereview.appspot.com/7528047.

R=fwereade, wallyworld
CC=
https://codereview.appspot.com/7782049

https://codereview.appspot.com/7782049/diff/4016/worker/provisioner/provision...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/7782049/diff/4016/worker/provisioner/provision...
worker/provisioner/provisioner_test.go:91: st, err := state.Open(o.Info,
state.DefaultDialTimeout)
On 2013/03/22 01:28:23, wallyworld wrote:
> Was using state.DefaultDialTimeout instead of TestingDialTimeout an oversight?

This wasn't so much an oversight as an attempt to make the old implicit
behaviour explicit. Because this code didn't live in the state/ package it could
not have access to the SetDialTimeout test helper. So, while we _thought_ that
because it was a test, it should have been using the shorter timeout, it was not
previously possible.

This is what jam was concerned about in my original proposal.

I'd like to leave this as is for the moment, as it doesn't change the previous
behaviour. It might be wrong, but at least it is consistently wrong, and we now
have a way to fix it in the future.