Code review comment for lp:~rogpeppe/gozk/allow-existing-server-dir

Revision history for this message
Roger Peppe (rogpeppe) wrote :

Reviewers: mp+96778_code.launchpad.net,

Message:
Please take a look.

Description:
It only succeeds if the directory is empty, for instance
when it's created by ioutil.TempDir.

Also add Server.Addr method and change the NotRunning
error to ErrNotRunning.

https://code.launchpad.net/~rogpeppe/gozk/allow-existing-server-dir/+merge/96778

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5787068/

Affected files:
   M runserver.go
   M server.go
   M server_test.go

Index: runserver.go
=== <email address hidden> >
<email address hidden>
=== modified file 'runserver.go'
--- runserver.go 2012-02-22 18:56:23 +0000
+++ runserver.go 2012-03-08 15:10:23 +0000
@@ -15,17 +15,19 @@
   "time"
  )

-var NotRunning = errors.New("process not running")
+// ErrNotRunning is the error returned when Process cannot
+// find the currently running zookeeper process.
+var ErrNotRunning = errors.New("process not running")

  // 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
-// NotRunning.
+// ErrNotRunning.
  func (srv *Server) Process() (*os.Process, error) {
   data, err := ioutil.ReadFile(srv.path("pid.txt"))
   if err != nil {
    if os.IsNotExist(err) {
- err = NotRunning
+ err = ErrNotRunning
    }
    return nil, err
   }
@@ -39,7 +41,7 @@
  // getProcess gets a Process from a pid and checks that the
  // process is actually running. If the process
  // is not running, then getProcess returns a nil
-// Process and the error NotRunning.
+// Process and the error ErrNotRunning.
  func getProcess(pid int) (*os.Process, error) {
   p, err := os.FindProcess(pid)
   if err != nil {
@@ -53,7 +55,7 @@
    return p, nil
   }
   if err == syscall.ESRCH {
- return nil, NotRunning
+ return nil, ErrNotRunning
   }
   return nil, errors.New("server running but inaccessible")
  }
@@ -65,7 +67,7 @@
    return err
   }
   p, err := srv.Process()
- if err == nil || err != NotRunning {
+ if err == nil || err != ErrNotRunning {
    if p != nil {
     p.Release()
    }
@@ -122,7 +124,7 @@
  func (srv *Server) Stop() error {
   p, err := srv.Process()
   if p == nil {
- if err != nil {
+ if err != nil && err != ErrNotRunning {
     return fmt.Errorf("cannot read process ID of server: %v", err)
    }
    return nil

Index: server.go
=== <email address hidden> >
<email address hidden>
=== modified file 'server.go'
--- server.go 2011-12-05 19:15:00 +0000
+++ server.go 2012-03-08 14:58:09 +0000
@@ -18,9 +18,10 @@
   zkDir string
  }

-// CreateServer creates the directory runDir and sets up a ZooKeeper server
-// environment inside it. It is an error if runDir already exists.
-// The server will listen on the specified TCP port.
+// CreateServer creates the directory runDir and sets up a ZooKeeper
+// server environment inside it. It is an error if runDir already
+// exists and is not empty. The server will listen on the specified TCP
+// port.
  //
  // The ZooKeeper installation directory is specified by zkDir.
  // If this is empty, a system default will be used.
@@ -28,7 +29,16 @@
  // CreateServer does not start the server.
  func CreateServer(port int, runDir, zkDir string) (*Server, error) {
   if err := os.Mkdir(runDir, 0777); err != nil {
- return nil, err
+ if !os.IsExist(err) {
+ return nil, err
+ }
+ info, err := ioutil.ReadDir(runDir)
+ if err != nil {
+ return nil, err
+ }
+ if len(info) != 0 {
+ return nil, fmt.Errorf("server directory %q is not empty")
+ }
   }
   srv := &Server{runDir: runDir, zkDir: zkDir}
   if err := srv.writeLog4JConfig(); err != nil {
@@ -88,6 +98,16 @@
   panic("not reached")
  }

+// Addr returns a local host address that can be used
+// to contact the server when it is running.
+func (srv *Server) Addr() (string, error) {
+ port, err := srv.networkPort()
+ if err != nil {
+ return "", err
+ }
+ return fmt.Sprintf("127.0.0.1:%d", port), nil
+}
+
  // command returns the command used to start the
  // ZooKeeper server.
  func (srv *Server) command() ([]string, error) {

Index: server_test.go
=== <email address hidden> >
<email address hidden>
=== renamed file 'reattach_test.go' => 'server_test.go'
--- reattach_test.go 2012-02-10 13:31:40 +0000
+++ server_test.go 2012-03-08 15:10:23 +0000
@@ -4,6 +4,7 @@
   "bufio"
   "flag"
   "fmt"
+ "io/ioutil"
   . "launchpad.net/gocheck"
   zk "launchpad.net/gozk/zookeeper"
   "os"
@@ -201,3 +202,47 @@
   err = srv.Stop()
   c.Assert(err, IsNil)
  }
+
+func (s *S) TestCreateServer(c *C) {
+ dir, err := ioutil.TempDir("", "zktest-")
+ c.Assert(err, IsNil)
+
+ zkdir := dir + "/zk"
+ // Check that it creates the new directory.
+ srv, err := zk.CreateServer(9999, zkdir, "")
+ c.Assert(err, IsNil)
+ c.Assert(srv, NotNil)
+
+ info, err := os.Stat(zkdir)
+ c.Assert(err, IsNil)
+ c.Assert(info.IsDir(), Equals, true)
+
+ addr, err := srv.Addr()
+ c.Assert(err, IsNil)
+ c.Assert(addr, Equals, "127.0.0.1:9999")
+
+ // Check that it fails when called again on the non-empty directory.
+ _, err = zk.CreateServer(9999, zkdir, "")
+ c.Assert(err, ErrorMatches, `server directory .* is not empty`)
+
+ // Check that Destroy removes the directory.
+ err = srv.Destroy()
+ c.Assert(err, IsNil)
+
+ _, err = os.Stat(zkdir)
+ if !os.IsNotExist(err) {
+ c.Errorf("expected not-exists error, got %v", err)
+ }
+
+ // Check that we can call CreateServer on the empty directory
+ srv, err = zk.CreateServer(8888, dir, "")
+ c.Assert(err, IsNil)
+ c.Assert(srv, NotNil)
+
+ addr, err = srv.Addr()
+ c.Assert(err, IsNil)
+ c.Assert(addr, Equals, "127.0.0.1:8888")
+
+ err = srv.Destroy()
+ c.Assert(err, IsNil)
+}

« Back to merge proposal