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

On 5 October 2011 15:34, Gustavo Niemeyer <email address hidden> wrote:
> Review: Approve
>
> 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.

done.

> +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.

ok, as discussed i'll just not bother about the race.

> [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.

better. my text was just wrong. done.
>
> Also, please mention that the data is preserved, and that the server can
> be restarted. "kill" may feel like otherwise.

done.

>
>
> [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.

done.

> [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.

i'm not sure what you mean here.

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

really? this code will have written the data inside the file, so there
should be no
extra space added. it's legal (ok it's pretty unusual) for a file name to
end in a space, and calling TrimSpace would break that.