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

PTAL

On 7 October 2011 16:58, Gustavo Niemeyer <email address hidden> wrote:
> [7]
>
> You've renamed installDir to zkDir. Let's rename functions and filenames
> to match that as well (zkdir.txt, writeZkDir, etc)

done

>
> [8]
>
> Ok, but this logic is wrong because it doesn't check the length and could
> panic.
>
> Alright, so instead of doing either of those things, please just don't append
> a '\n' at the end, so we can just string(data) straight. This is already
> being done for pid.txt, and sounds like a sane approach.

done

> [11]
>
> +// ServerCommand returns the command used to start the
> +// ZooKeeper server. It is provided for debugging and testing
> +// purposes only.
> +func (srv *Server) command() ([]string, os.Error) {
>
> s/ServerCommand/command/
> s/It is provided.*//

given that it's not external any more, that "debugging and testing"
remark isn't necessary now.

> [12]
>
> +// NetworkPort returns the TCP port number that
> +// the server is configured for.
>
> s/NetworkPort/networkPort/
>
> [13]
>
> +// This file defines methods on Server that deal with starting
> +// and stopping the ZooKeeper service. They are independent of ZooKeeper
> +// itself, and may be factored out at a later date.
>
> Please merge this on server.go. As pointed out in the other review and obvious
> through out the code, this is really specific to ZooKeeper. If/when we do
> refactor this out to generalize, we'll have to reorganize anyway.

i can do this, but given that the dependencies of the two files
are mostly different (only fmt, ioutil and os in common), and there's
very little interlink between the two, i think it makes sense to keep
them as two different files - i think it makes the code easier to read.
perhaps "runserver.go" might be a better name than "service.go" though.
i've done that for the time being. that said, it's no big deal.

> [14]
>
> +var ErrNotRunning = os.NewError("process not running")
>
> I'm not a big fan of the Err* convention. Makes for an ugly name with typing
> information embedded on it. Some packages do use it, but thankfully it's
> also not strictly followed in the Go standard library either.
>
> I'd appreciate renaming this to NotRunning instead. zk.NotRunning seems
> pretty good.

ok.

>
> [15]
>
> +// Process returns a Process referring to the running server from
> +// where it's been stored in pid.txt. If the file does not
> +// exist, or it cannot find the process, it returns the error
> +// ErrNotRunning.
>
> Where the pid is stored is an implementation detail that can
> change. We don't have to document that for now.
>
> [16]
>
> +// getProcess gets a Process from a pid and check that the
>
> s/check/checks/

done

> [18]
>
> +               // is to poll until it exits. If the process has taken longer than
> +               // a second to exit, then it's probably not going to.
>
> This is very optimistic. A process can easily take more than a second to exit. If
> this is an edge case and there's a reason to wait at all (dir content, etc), then
> let's wait a bit longer, like 10 seconds for instance.

ok. given that we're killing the server with SIGKILL, it's not going to
have any chance to take ages to clean up. maybe we should go for
a lighter touch and add some heuristics to do a SIGKILL after some time
has elapsed. yuk. i've changed it to 5 seconds for the time being, as 10s
seems like overkill.