state: add Info type and make Open use it

https://code.launchpad.net/~rogpeppe/juju/go-state-info/+merge/92321

(do not edit description out of merge proposal)

[rog, 2012-02-09 17:06:16]

Please take a look.

[TheMue, 2012-02-09 20:41:27]

https://codereview.appspot.com/5655043/diff/1/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5655043/diff/1/state/export_test.go#newcode1
state/export_test.go:1: package state
What is the idea behind this test file and why is it called export_test.go? Is
it a whitebox test like the tests in internal_test.go or a blackbox test like
state_test.go? Also the header comment is missing.

https://codereview.appspot.com/5655043/diff/1/state/export_test.go#newcode8
state/export_test.go:8: func OpenAddr(c *C, addr string) (st *State, zk
*zookeeper.Conn) {
This seems to be a helper. It should be commented why it's there. Why is it
public?

https://codereview.appspot.com/5655043/diff/1/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5655043/diff/1/state/internal_test.go#newcode55
state/internal_test.go:55: _, s.zkConn = OpenAddr(c, s.zkAddr)
Why not state.Open() here?

https://codereview.appspot.com/5655043/diff/1/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/1/state/open.go#newcode14
state/open.go:14: Addrs []string
How about using net.IPAddress here? Would be a nice typesafe way.

https://codereview.appspot.com/5655043/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5655043/diff/1/state/state_test.go#newcode64
state/state_test.go:64: s.st, s.zkConn = state.OpenAddr(c, s.zkAddr)
Why not using state.Open() here?

[rog, 2012-02-10 09:02:39]

Please take a look.

[rog, 2012-02-10 09:03:51]

PTAL

https://codereview.appspot.com/5655043/diff/1/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5655043/diff/1/state/export_test.go#newcode1
state/export_test.go:1: package state
On 2012/02/09 20:41:27, TheMue wrote:
> What is the idea behind this test file and why is it called export_test.go? Is
> it a whitebox test like the tests in internal_test.go or a blackbox test like
> state_test.go? Also the header comment is missing.

as discussed in IRC, it's not a test, it's the conventional way of exposing some
extra information not usually available outside the package.

https://codereview.appspot.com/5655043/diff/1/state/export_test.go#newcode8
state/export_test.go:8: func OpenAddr(c *C, addr string) (st *State, zk
*zookeeper.Conn) {
On 2012/02/09 20:41:27, TheMue wrote:
> This seems to be a helper. It should be commented why it's there. Why is it
> public?

it's here so that tests can have access to the direct zookeeper connection as
well as the State itself. Putting the  Open and the checks in here saves a few
lines of boilerplate in three places.

https://codereview.appspot.com/5655043/diff/1/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5655043/diff/1/state/internal_test.go#newcode55
state/internal_test.go:55: _, s.zkConn = OpenAddr(c, s.zkAddr)
On 2012/02/09 20:41:27, TheMue wrote:
> Why not state.Open() here?

because state.Open does not return a zk connection.

https://codereview.appspot.com/5655043/diff/1/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/1/state/open.go#newcode14
state/open.go:14: Addrs []string
On 2012/02/09 20:41:27, TheMue wrote:
> How about using net.IPAddress here? Would be a nice typesafe way.

net.IPAddress isn't appropriate, as we want to use the unresolved name rather
than the resolved IP address, and we want to include the port number.

The form of an address is exactly that expected by net.Dial.
I'll add a comment to that effect.

https://codereview.appspot.com/5655043/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5655043/diff/1/state/state_test.go#newcode64
state/state_test.go:64: s.st, s.zkConn = state.OpenAddr(c, s.zkAddr)
On 2012/02/09 20:41:27, TheMue wrote:
> Why not using state.Open() here?

because we need the zk connection so we can call zkRemoveTree on it.

[fwereade, 2012-02-10 12:07:42]

https://codereview.appspot.com/5655043/diff/2003/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode17
state/open.go:17: Addrs []string
I'm not totally wild about turning a comma-separated list of addresses into a
list of strings only to turn them back into a comma-separated list again; I
guess the intent is to avoid exposing zookeepericity to clients?

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode33
state/open.go:33: }
Should we be doing something like:

go func() {
  for {
    <-session
  }
}

...here? We don't know how long the State is going to be around for, and I think
we've set a timebomb for ourselves by leaving the session event buffer filling
up with no release valve.

(I don't *think* we need to pay any attention to those events, the critical ones
will happen over other channels as well.)

[niemeyer, 2012-02-10 12:12:26]

https://codereview.appspot.com/5655043/diff/2003/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode10
state/open.go:10: // Info encapsulates information about a juju state server
s/state server or servers/ZooKeeper cluster/

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode12
state/open.go:12: // that state.
s/state/cluster/

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode14
state/open.go:14: // Addrs gives the addresses of the zookeeper
s/zookeeper/ZooKeeper/

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode16
state/open.go:16: // expected by net.Dial (i.e. address:port).
s/expected by net.Dial (i.e. address:port)/address:port/

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode27
state/open.go:27: zk, session, err := zookeeper.Dial(strings.Join(info.Addrs,
","), 5e9)
s/5e9/zkTimeout/, and define it above as

const zkTimeout = 15e9

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode31
state/open.go:31: if !(<-session).Ok() {
We'll need that channel later, to monitor the state of the connection with
ZooKeeper, but it's ok for now.

[rog, 2012-02-10 12:22:37]

Please take a look.

[rog, 2012-02-10 12:23:14]

https://codereview.appspot.com/5655043/diff/2003/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode17
state/open.go:17: Addrs []string
On 2012/02/10 12:07:42, fwereade wrote:
> I'm not totally wild about turning a comma-separated list of addresses into a
> list of strings only to turn them back into a comma-separated list again; I
> guess the intent is to avoid exposing zookeepericity to clients?

no, that's not the intent (the clients know that they're creating an info with a
bunch of zk addresses in).

i consider the fact that zk dial accepts a comma-separated set of addresses to
be an implementation detail. it makes things easier if we hold them as a slice
internally - we have to separate them out if we want to do ssh proxying, for
example. and i think it's cleaner too.

i don't think the addresses will start life as a comma separated list either,
when the bootstrapping process starts up those machines.

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode33
state/open.go:33: }
On 2012/02/10 12:07:42, fwereade wrote:
> Should we be doing something like:
> 
> go func() {
>   for {
>     <-session
>   }
> }
> 
> ...here? We don't know how long the State is going to be around for, and I
think
> we've set a timebomb for ourselves by leaving the session event buffer filling
> up with no release valve.
> 
> (I don't *think* we need to pay any attention to those events, the critical
ones
> will happen over other channels as well.)

yes, probably. i've not found a use for the global session events yet, but i'm
sure someone has...
done.

[TheMue, 2012-02-10 12:31:09]

https://codereview.appspot.com/5655043/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/6001/state/open.go#newcode17
state/open.go:17: Addrs []string
How about an own type Addresses []string for it. It will be created by
ParseAddresses(string) (Addresses, error) and can be returned as string by
(Addresses) String() string.

[niemeyer, 2012-02-10 12:36:12]

https://codereview.appspot.com/5655043/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/6001/state/open.go#newcode34
state/open.go:34: // throw away subsequent session events.
Please remove that. Those events are relevant, and gozk will correctly panic if
the buffer fills up. There are two possibilities here: handle the errors, or
code the logic in such a way that they do not matter. In both cases the buffer
should not fill up. I want to hear if it does.

[rog, 2012-02-10 12:48:30]

Please take a look.

[rog, 2012-02-10 12:48:43]

https://codereview.appspot.com/5655043/diff/2003/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode10
state/open.go:10: // Info encapsulates information about a juju state server
On 2012/02/10 12:12:26, niemeyer wrote:
> s/state server or servers/ZooKeeper cluster/

done, except i'd prefer to keep mention of Zookeeper
out of this comment - if you're just using the Info
type rather than constructing it, it's zk-agnostic.

changed to:


// Info encapsulates information about cluster of
// servers holding juju state and can be used to make a
// connection to that cluster.

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode12
state/open.go:12: // that state.
On 2012/02/10 12:12:26, niemeyer wrote:
> s/state/cluster/

Done.

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode14
state/open.go:14: // Addrs gives the addresses of the zookeeper
On 2012/02/10 12:12:26, niemeyer wrote:
> s/zookeeper/ZooKeeper/

Done.

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode16
state/open.go:16: // expected by net.Dial (i.e. address:port).
On 2012/02/10 12:12:26, niemeyer wrote:
> s/expected by net.Dial (i.e. address:port)/address:port/

Done.

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode27
state/open.go:27: zk, session, err := zookeeper.Dial(strings.Join(info.Addrs,
","), 5e9)
On 2012/02/10 12:12:26, niemeyer wrote:
> s/5e9/zkTimeout/, and define it above as
> 
> const zkTimeout = 15e9

Done.

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode31
state/open.go:31: if !(<-session).Ok() {
On 2012/02/10 12:12:26, niemeyer wrote:
> We'll need that channel later, to monitor the state of the connection with
> ZooKeeper, but it's ok for now.

won't the watches be enough to get that information without this channel?

https://codereview.appspot.com/5655043/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/6001/state/open.go#newcode17
state/open.go:17: Addrs []string
On 2012/02/10 12:31:09, TheMue wrote:
> How about an own type Addresses []string for it. It will be created by
> ParseAddresses(string) (Addresses, error) and can be returned as string by
> (Addresses) String() string.

if string is good enough for net.Dial, []string is good enough here, IMHO.

https://codereview.appspot.com/5655043/diff/6001/state/open.go#newcode34
state/open.go:34: // throw away subsequent session events.
On 2012/02/10 12:36:12, niemeyer wrote:
> Please remove that. Those events are relevant, and gozk will correctly panic
if
> the buffer fills up. There are two possibilities here: handle the errors, or
> code the logic in such a way that they do not matter. In both cases the buffer
> should not fill up. I want to hear if it does.

the errors will arrive on watches too - i'm not sure how much value a global
session event channel adds.
if there are no watches, we don't care about the state of the server until we
actually come to do anything AFAICS.

i've removed the loop for the time being.

[niemeyer, 2012-02-10 13:05:58]

LGTM

https://codereview.appspot.com/5655043/diff/2003/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/2003/state/open.go#newcode31
state/open.go:31: if !(<-session).Ok() {
On 2012/02/10 12:48:44, rog wrote:
> won't the watches be enough to get that information without this channel?

The watches will get errors in their own context. If they will be enough, maybe,
but we'll know when we get all agents working correctly. That's why this is ok
for now.

https://codereview.appspot.com/5655043/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/5655043/diff/6001/state/open.go#newcode34
state/open.go:34: // throw away subsequent session events.
On 2012/02/10 12:48:44, rog wrote:
> the errors will arrive on watches too - i'm not sure how much value a global
> session event channel adds.

If nothing else, it's a trivial way to monitor the server state. If we have a
loop holding on events, we may as well reestablish broken connections
pro-actively rather than waiting for our own requests to blow up.

[rog, 2012-02-10 13:49:29]

*** Submitted:

state: add Info type and make Open use it

R=TheMue, fwereade, niemeyer
CC=
https://codereview.appspot.com/5655043