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

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" really mean, and
is localhost always ok?
On 2014/03/31 12:22:11, jameinel wrote:
> origin would have sense if you were doing actual web requests, I think
it is the
> location that redirected you here.
> I believe the websocket requires something, but it accepts localhost,
so we just
> do it.

> It isn't actually used in our implementation, so it is ok.

> See something like:

http://learnitcorrect.com/blog/websocket-is-great-but-not-the-origin-policy.html

> It is intended to avoid cross-site scripting, etc. But we just let
people
> connect to our websockets, and do the auth inside the websocket (as
part of
> Login) rather than assuming that connections to us are already valid.

I just moved that TODO, but thanks for the info :)
I'll remove the TODO, and update the comment.

https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go#newcode167
state/api/apiclient.go:167: return conn, nil
On 2014/03/31 12:22:11, jameinel wrote:
> Would we want to check <-stop at this point, in case we got a stop
while the
> dial was processing?

> I guess it has to be handled up a layer anyway. We just know that
DialConfig is
> blocking, so we sort of expect to be in the middle of dialing while we
get a
> stop request. (because someone else succeeded.)

It is indeed handled higher up; utils/parallel.Try.Start creates a
closure that will dispose of the result by Close()ing it once "stop"
("dying" inside Try) is closed.

https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go#newcode169
state/api/apiclient.go:169: log.Debugf("state/api: %v", err)
On 2014/03/31 12:22:11, jameinel wrote:
> maybe a little more context about "error trying to Dial API server,
will retry:
> %v" ?

> It depends what errors we generally get, but it seems helpful to give
a bit more
> context than just an error.

Done.

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"
On 2014/03/31 11:55:23, dimitern wrote:
> d

Done.

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