Code review comment for lp:~axwalk/juju-core/client-cache-apihostports

Please take a look.

https://codereview.appspot.com/81780043/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode252
juju/api.go:252: logger.Warningf(err.Error())
On 2014/03/28 10:02:12, rog wrote:
> This isn't right (the error might contain % characters).

> logger.Warningf("Cannot cache cache API addresses: %v", err)

> (and change the error returned from cacheChangedAPIAddresses
> so that the message won't be repetitious)

I copied that from newAPIFromStore and clearly I didn't look too close.
Fixed both.

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode340
juju/api.go:340: endpoint.Addresses = addrs
On 2014/03/28 10:02:12, rog wrote:
> I'd like to add a HostPorts entry to the APIEndpoint struct
> (we could perhaps deprecate the Addresses field)
> and store all the APIHostPorts info there.

> So all the code here would deal only with instance.HostPort,
> not address strings. This leaves us free to have clients
> that are more intelligent about dialling in the future.

+1

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode356
juju/api.go:356: sort.Strings(a)
On 2014/03/28 10:02:12, rog wrote:
> I don't think I'd sort the addresses - the order can be significant.

Done.

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

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go#newcode35
state/api/apiclient.go:35: addrs []string
On 2014/03/28 10:02:12, rog wrote:
> I think it would probably be better to store the hostports exactly as
returned
> by APIHostPorts here, leaving it up to client code to decide how best
to use the
> information.

Done.

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go#newcode206
state/api/apiclient.go:206: func (s *State) Addrs() []string {
On 2014/03/28 10:02:12, rog wrote:
> I'd rename this HostPorts and return all
> the APIHostPorts results, with the connecting
> address added as an extra element if it's
> not found in the result.

Done. If the connecting address isn't found, it's added with
NetworkUnknown scope.

> Also, I wonder if this logic wouldn't live more naturally
> inside the Login method.

Yeah, it does, because there's a theoretical error when converting
s.addr->HostPort. Moved.

https://codereview.appspot.com/81780043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/params/params.go#newcode645
state/api/params/params.go:645: APIHostPortsResult
On 2014/03/28 10:02:12, rog wrote:
> I think I'd just include the field explicitly here
> rather than embedding.

Done.

https://codereview.appspot.com/81780043/diff/1/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/state.go#newcode35
state/api/state.go:35: // The client must attempt *all* addresses, or
else
On 2014/03/28 10:02:12, rog wrote:
> This comment seems like it might be more helpful to put as part of the
Addrs (or
> HostPorts) doc comment.

Done.

https://codereview.appspot.com/81780043/diff/1/state/apiserver/admin.go
File state/apiserver/admin.go (right):

https://codereview.appspot.com/81780043/diff/1/state/apiserver/admin.go#newcode88
state/apiserver/admin.go:88: // state; failure here is non-fatal.
On 2014/03/28 10:02:12, rog wrote:
> I think it's perfectly reasonable to make the error fatal here.
> If we can't access the APIHostPorts document, something is seriously
> wrong.

Fair enough, done. I just worry about Login being rendered unusable by a
bug, meaning upgrades are broken and we can't fix anything. This is a
bigger problem than this one case, though.

https://codereview.appspot.com/81780043/