Code review comment for lp:~rogpeppe/gozk/update-server-interface

Great stuff here Rog, thanks!

Some comments:

[1]

-// The ZooKepeer installation directory is specified by installedDir.
+// The ZooKeeper installation directory is specified by installDir.

installDir feels like a strange name in this context. I agree installedDir
isn't great either, but the new one gives a wrong impression about what
it means.. we're not installing anything there.

Let's use something like zkDir instead.

[2]

+func CreateServer(port int, runDir, installDir string) (*Server, os.Error) {
(...)
+func AttachServer(runDir string) (*Server, os.Error) {

The organization inside these is looking very nice.

[3]

+func (srv *Server) getServerProcess() (*os.Process, os.Error) {
(...)
+ return os.FindProcess(pid)

FindProcess does nothing on Unix. We can run at least a Signal 0 against
the pid to tell if it exists or not.

[4]

+ if p != nil {
+ p.Release()
+ }
+ return fmt.Errorf("ZooKeeper server may already be running (remove %q to clear)", srv.path("pid.txt"))

The recommendation here is a bit strange. If the process is actually running,
removing the pid file will allow a second server to be started in parallel,
which is not nice.

I suggest just saying "ZooKeeper server seems to be running already", which
is a bit more true given that we'll be checking the pid as per the point above.
Then, the developer can run Stop() on it if he feels like stopping it, or
do whatever else out of band if he doesn't want to use the interface, but
then it's his responsibility to check for background processes, etc.

[5]

+// Stop kills the ZooKeeper server. It is a no-op if it is already running.

The second part is reversed, and reads a bit hard.

Suggestion: It does nothing if not running.

Also, please mention that the data is preserved, and that the server can
be restarted. "kill" may feel like otherwise.

[6]

+// Destroy stops the ZooKeeper server, and then removes its run
+// directory and all its contents.

s/and all its contents././

Removing the directory means removing its contents necessarily.

[7]

+func (srv *Server) writeInstallDir() os.Error {
+ return ioutil.WriteFile(srv.path("installdir.txt"), []byte(srv.installDir+"\n"), 0666)

These functions and filename should be renamed in sync to the first point.

[8]

+ if data[len(data)-1] == '\n' {
+ data = data[0 : len(data)-1]
+ }
+ srv.installDir = string(data)

srv.zkDir = string(bytes.TrimSpace(data))

[9]

+ s.zkTestRoot = c.MkDir() + "/zk"
+ port := 21812
+ s.zkAddr = fmt.Sprint("localhost:", port)
+
+ s.zkServer, err = zk.CreateServer(port, s.zkTestRoot, "")
  if err != nil {
   c.Fatal("Cannot set up server environment: ", err)
  }
- s.StartZK(c)
+ err = s.zkServer.Start()
+ if err != nil {
+ c.Fatal("Cannot start ZooKeeper server: ", err)
+ }

This is awesome! Thanks!