Merge lp://staging/~rogpeppe/gozk/factor-out-service into lp://staging/~juju/gozk/trunk

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

On 5 October 2011 16:32, Gustavo Niemeyer <email address hidden> wrote:
> Review: Disapprove
>
> 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.

ok, i'll do that if you don't mind.

the thing that was mostly bothering me was the mingling of the code
from Server and Service, and that the complexities of Service were
obscuring the essential simplicity of Server.

BTW many of the comments below are applicable even without
the factored-out service package.

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

oh, i thought it was in the earlier merge.

i've added to the update-server-interface, if that's ok.

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

actually maybe RunDir

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

perhaps. i preferred to keep the availability checking separate
so that calling Command didn't imply that the service was
going to be started immediately.

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

it doesn't guarantee it, but it does mean that we can return
an early error rather than relying on the server to print
the error somewhere at some unspecified later time.
i found it very useful when testing.

i wonder if the Start should print stdout and stderr from
the server for the first half second or so after it's been started.
that's a possible solution, but makes things slower.

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

my plan was to change things so the server is started in
its own process group or sess...