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

Please take a look.

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go#newcode160
juju/apiconn_test.go:160: // be updated (to an empty slice).
On 2014/03/31 08:37:33, rog wrote:
> As discussed online, I'm not sure that this is the correct approach -
we
> shouldn't rely on calling methods on types that we've created with
New.

Done.

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

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode212
state/api/apiclient.go:212: func (s *State) HostPorts()
[][]instance.HostPort {
On 2014/03/31 08:37:33, rog wrote:
> On further reflection, I think this method should be named
APIHostPorts,
> mirroring the same function in State, and making it hopefully clearer
what the
> returned addresses are about.

Done.

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode213
state/api/apiclient.go:213: return append([][]instance.HostPort{},
s.hostPorts...)
On 2014/03/31 08:37:33, rog wrote:
> If you're serious about returning a copy, then you should probably
copy all the
> sub-slices too.

> Otherwise, just return s.hostPorts and document that the returned
slice should
> not be modified.

Hah, thanks for that. Fixed.

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

https://codereview.appspot.com/81780043/diff/20001/state/api/state.go#newcode40
state/api/state.go:40: if err := st.addAddrHostPort(); err != nil {
On 2014/03/31 08:37:33, rog wrote:
> rather than have addAddrHostPort be a method that mutates
st.hostPorts, perhaps
> it might be clearer to have something like:

> hostPorts, err := addAddress(result.Servers, st.addr)
> if err != nil {
> st.Close()
> return err
> }
> st.hostPorts = hostPort

> // addAddress appends a new server derived from the given
> // address to servers if the address is not already found
> // there.
> func addAddress(servers [][]instance.HostPort, addr string)
> ([][]instance.HostPort, error)

Done.

> Then addAddress is easily testable if you want to.

> BTW I have mixed feelings about returning an error if we can't split
the dial
> address. It means that if the dial address isn't parseable for some
reason (e.g.
> the port is non-numeric) we'll fail the connection, even though we
actually
> managed to connect.

Me too, but at the moment that remains theoretical. I'd rather have it
fail spectacularly if/when we decide to have a non-numeric
/etc/services-style port, and fix it then.

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