Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(740)

Issue 82450043: state/api: dial info.Addrs in parallel

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years ago by axw
Modified:
9 years, 12 months ago
Reviewers:
dimitern, jameinel, mp+213401, rog
Visibility:
Public.

Description

state/api: dial info.Addrs in parallel This change is to dial all addresses in parallel. The first one that successfully connects is used. https://code.launchpad.net/~axwalk/juju-core/apiclient-open-parallel/+merge/213401 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8

Patch Set 2 : state/api: dial info.Addrs in parallel #

Patch Set 3 : state/api: dial info.Addrs in parallel #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+143 lines, -29 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M state/api/apiclient.go View 1 2 3 chunks +59 lines, -26 lines 8 comments Download
A state/api/apiclient_test.go View 1 1 chunk +79 lines, -0 lines 0 comments Download
M state/api/state.go View 1 2 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 7
axw
Please take a look.
10 years ago (2014-03-31 04:15:32 UTC) #1
dimitern
LGTM https://codereview.appspot.com/82450043/diff/1/state/api/apiclient_test.go File state/api/apiclient_test.go (right): https://codereview.appspot.com/82450043/diff/1/state/api/apiclient_test.go#newcode10 state/api/apiclient_test.go:10: //jc "github.com/juju/testing/checkers" d
10 years ago (2014-03-31 11:55:23 UTC) #2
jameinel
A couple of comments, but nothing that would block landing this. LGTM https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go File state/api/apiclient.go ...
9 years, 12 months ago (2014-03-31 12:22:10 UTC) #3
axw
Please take a look. https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go File state/api/apiclient.go (right): https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go#newcode142 state/api/apiclient.go:142: // TODO what does "origin" ...
9 years, 12 months ago (2014-03-31 13:12:13 UTC) #4
axw
Please take a look.
9 years, 12 months ago (2014-03-31 14:47:28 UTC) #5
rog
A few fixes that would be nice to land when possible. https://codereview.appspot.com/82450043/diff/40001/state/api/apiclient.go File state/api/apiclient.go (right): ...
9 years, 12 months ago (2014-03-31 15:39:01 UTC) #6
axw
9 years, 12 months ago (2014-04-01 03:25:02 UTC) #7
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#new...
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#new...
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#new...
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#new...
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b