Code review comment for lp:~axwalk/juju-core/apiclient-open-parallel

Updates over here: https://codereview.appspot.com/82900045/

https://codereview.appspot.com/82450043/diff/40001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/82450043/diff/40001/state/api/apiclient.go#newcode117
state/api/apiclient.go:117: if err := dialWebsocket(addr, opts, pool,
try); err != nil {
On 2014/03/31 15:39:01, rog wrote:
> This doesn't seem quite right. Try.Start returns ErrStopped if an
earlier
> attempt has succeeded. If that happens, we want to stop calling
dialWebsocket.

> I think something like this might be better:

> for _, addr := range info.Addrs {
> err := dialWebsocket(addr, opts, pool, try)
> if err == parallel.ErrStopped {
> break
> }
> if err != nil {
> return nil, err
> }
> select{
> case <-time.After(dialAddressInterval):
> case <-try.Dead():
> }
> }

> with dialAddressInterval being something like 50ms,
> so if the connect succeeds fast, we can avoid dialing
> most of the addresses.

Done.

https://codereview.appspot.com/82450043/diff/40001/state/api/apiclient.go#newcode168
state/api/apiclient.go:168: err := parallel.ErrStopped
On 2014/03/31 15:39:01, rog wrote:
> This isn't quite right - if we used up all our attempts, we probably
want to
> return a timeout error rather than ErrStopped.

> I quite like using HasNext for this kind of thing:

> for a := openAttempt.Start(); a.Next(); {
> select{
> case <-stop:
> return nil, parallel.ErrStopped
> default:
> }
> conn, err := websocket.DialConfig(...)
> if err == nil {
> return conn, nil
> }
> if !a.HasNext() {
> return nil, fmt.Errorf("timed out connecting to %q",
cfg.Location)
> }
> }
> panic("unreachable")

Done.

https://codereview.appspot.com/82450043/diff/40001/state/api/apiclient.go#newcode169
state/api/apiclient.go:169: for a := openAttempt.Start(); a.Next(); {
On 2014/03/31 15:39:01, rog wrote:
> It's a pity we don't have a method on Attempt that lets us use
> it in selects, so that it could be interrupted immediately.

> At some point, something like:

> // NextDuration reports the interval until the
> // next attempt should be made.
> func (a *Attempt) NextDuration() time.Duration

> might be good. Then you could write:

> for a := openAttempt.Start(); a.HasNext(); {
> select {
> case <-a.stop:
> return nil, parallel.ErrStopped
> case <-time.After(a.NextDuration()):
> }
> ....
> }

I looked briefly, but it looks a little complicated. I'll not attempt
(ha ha) this for the moment.

https://codereview.appspot.com/82450043/diff/40001/state/api/apiclient.go#newcode172
state/api/apiclient.go:172: break
On 2014/03/31 15:39:01, rog wrote:
> this is a no-op.
> I think you probably want:

> return nil, parallel.ErrStopped

> It could probably do with a test.

/facepalm
thanks, fixed and added a test.

https://codereview.appspot.com/82450043/