Code review comment for lp:~rogpeppe/gozk/clean-up-interface

This is looking fantastic. Thanks Roger.

Some details to sort out:

[1]

=== renamed file 'gozk.go' => 'zookeeper/gozk.go'

Please keep the files at the root of the branch. We'll put it in a
new series in launchpad so that the gozk/zookeeper reference is
correct.

[2]

+// Conn represents a connection to a set of Zookeeper nodes.

The project name is "ZooKeeper". Please use this casing consistently
across the code base.

[3]

+// ClientId represents an established ZooKeeper session. It can be
+// passed into New to reestablish a connection to an existing session.

There's no New function.

[4]

+ ZOK Error = C.ZOK
+ ZSYSTEMERROR Error = C.ZSYSTEMERROR
+ ZRUNTIMEINCONSISTENCY Error = C.ZRUNTIMEINCONSISTENCY

Much better!

[5]

+func (zk *Conn) RetryChange(path string, flags int, acl []ACL, changeFunc ChangeFunc) os.Error {

Very nice changes in RetryChange too, thanks!

[6]

+ path, err := zk.Create("/test-", "bababum", zookeeper.SEQUENCE|zookeeper.EPHEMERAL, zookeeper.WorldACL(zookeeper.PERM_ALL))

These lines make me unhappy about the length of "zookeeper". I'm thinking we
might well rename it to "zk" in a follow up branch, but please keep it like
this for now. Just for comparison:

path, err := conn.Create("/test-", "bababum", zk.SEQUENCE|zk.EPHEMERAL, zk.WorldACL(zk.PERM_ALL))

[7]

+ c.Assert(stat, Equals, (*zookeeper.Stat)(nil))

c.Assert(stat, IsNil)

[8]

+// Server sets up a zookeeper server environment inside dataDir

ZooKeeper in this file as well.

[9]

+// for a server that listening on the specified TCP port, sending

s/listening/listens/

[10]

+// Server does not actually start the server. Instead it returns
+// a command line, suitable for passing to exec.Command,
+// for example.

Let's do a bit better than this. There's no reason to keep the
responsibility of creating the directory, starting, stopping,
cleaning, etc, with the caller. We need this for real, not just
due to testing, but in juju for the local deployment case.

I suggest we define a Server type as:

  type Server struct {
      runDir, installDir string
      cmd *exec.Cmd
  }

and:

  func zookeeper.NewServer(runDir) *Server

Then, we should have this as well:

  func (s *Server) Create(installDir string) os.Error
  func (s *Server) Start() os.Error
  func (s *Server) Stop() os.Error
  func (s *Server) Destroy() os.Error

Please push this in a spearate merge proposal. I'm happy to
merge the current version as is for the moment.

[11]

+var log4jProperties = []byte(`

No need to have two copies of that file in memory at all times. Use a const
and []byte() it when necessary.

[12]

+ classPath, _ := filepath.Glob(filepath.Join(dir, "zookeeper-*.jar"))
+ more, _ := filepath.Glob(filepath.Join(dir, "lib/*.jar"))

Please check the errors.

[13]

+ fd, err := os.Open(zookeeperEnviron)

That's a file, or an f, not an fd.

[14]

+ line, err := r.ReadSlice('\n')

ReadLine()

[15]

+ // strip quotes
+ if path[0] == '"' {
+ path = path[1 : len(path)-1]
+ }

Please use Trim. This logic assumes it knows the position of quotes.