Code review comment for lp:~rogpeppe/gozk/factor-out-service

This is an interesting idea, but at the same time it's full of problems and
not really necessary right now. I suggest we delay it until a future point
when we actually need that abstraction, as we'll be in a better position
to generalize it. At that point, we can even preserve for the internal
implementation of the ZooKeeper server itself, without many problems.

Here is some feedback, regardless, which I suggest you don't work on
right now besides for [1] which should be integrated.

[1]

=== added file 'reattach_test.go'
--- reattach_test.go 1970-01-01 00:00:00 +0000
+++ reattach_test.go 2011-10-05 14:37:11 +0000

This should be in a separate merge proposal as it's important and unrelated
to the strawman changes proposed.

[2]

+type Interface interface {
+ // Directory gives the path to the directory containing
+ // the server's configuration and data files. It is assumed that
+ // this exists and can be modified.
+ Directory() string

s/Directory/Dir/

[3]

+
+ // CheckAvailability should return an error if resources
+ // required by the server are not available; for instance
+ // if the server requires a network port which is not free.
+ CheckAvailability() os.Error

We don't need this. The Command function can return an error if
it knows it cannot start.

[4]

+ l, err := net.Listen("tcp", fmt.Sprintf("localhost:%d", port))
+ if err != nil {
+ return fmt.Errorf("cannot listen on port %v: %v", port, err)
+ }
+ l.Close()

This doesn't really guarantee that when the service itself is started,
the port will be available, so we can as well not do it.

[5]

+// Process returns a reference to the identity
+// of the last running server in the Service's directory.
+// If the server was shut down, it returns (nil, nil).

This shouldn't be in a generic service API. What if there
are multiple processes handled by the service?

[6]

+// If the server was shut down, it returns (nil, nil).

If we're returning a nil *Process, we necessarily need err != nil.

[7]

+// It stores the process ID of the server in the file "pid.txt"
+// inside the server's directory. Stdout and stderr of the server
+// are similarly written to "log.txt".

That's not nice. Who owns the content of this directory, the
interface implementation, or the Service? We can't write
arbitrary content into a directory of different ownership.

Also, a generic interface would need to do better in terms
of logging. We could have a file somewhere named output.txt, maybe,
and let logging with the underlying implementation.

[8]

+ if err := p.Kill(); err != nil && !strings.Contains(err.String(), "no such process") {
+ return fmt.Errorf("cannot kill server process: %v", err)
+ }

Different services are stopped in different ways. Some prefer a SIGTERM,
before a SIGKILL, for instance. And what happens if there are multiple
processes?

[9]

+ if err != nil && strings.Contains(err.String(), "no child processes") {
+ // If we can't wait for the server, it's possible that it was running
+ // but not as a child of this process, so give it a little while
+ // to exit. TODO poll with kill(no signal)?
+ time.Sleep(0.5e9)
+ }

Comparing string errors like this is really bad. There should be
an error code that we can check. Then, yeah, a sleep isn't a nice
approach.

[10]

  if s.zkServer != nil {
- s.zkServer.Destroy()
+ s.zkServer.Stop() // TODO Destroy
  }

This must be done.

[11]

Where are the tests for the service?