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?