gozk

Merge lp:~rogpeppe/gozk/allow-existing-server-dir into lp:gozk/zookeeper

  1. allow-existing-server-dir
  2. Merge into zookeeper
Proposed by Roger Peppe
Status: Merged
Approved by: Gustavo Niemeyer
Approved revision: 34
Merged at revision: 32
Proposed branch: lp:~rogpeppe/gozk/allow-existing-server-dir
Merge into: lp:gozk/zookeeper
Diff against target: 175 lines (+78/-11)
3 files modified
runserver.go (+9/-7)
server.go (+24/-4)
server_test.go (+45/-0)
To merge this branch: bzr merge lp:~rogpeppe/gozk/allow-existing-server-dir
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+96778@code.launchpad.net

Description of the change

zookeeper: allow CreateServer to work in an existing directory.

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://codereview.appspot.com/5787068/

To post a comment you must log in.
Revision history for this message
Roger Peppe (rogpeppe) wrote :
Download full text (5.8 KiB)

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

Read more...

Revision history for this message
William Reade (fwereade) wrote :

LGTM

https://codereview.appspot.com/5787068/diff/5/runserver.go
File runserver.go (right):

https://codereview.appspot.com/5787068/diff/5/runserver.go#newcode20
runserver.go:20: var ErrNotRunning = errors.New("process not running")
I'm not really *against* this change, but I don't understand the
motivation -- is the Err-ness not always clear from context?

https://codereview.appspot.com/5787068/

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

forgot to publish this.

https://codereview.appspot.com/5787068/diff/5/runserver.go
File runserver.go (right):

https://codereview.appspot.com/5787068/diff/5/runserver.go#newcode20
runserver.go:20: var ErrNotRunning = errors.New("process not running")
On 2012/03/12 10:34:46, fwereade wrote:
> I'm not really *against* this change, but I don't understand the
 motivation --
> is the Err-ness not always clear from context?

it's conventional. try a recursive grep in $GOROOT for
'^var.*errors\.New'

there are one or two exceptions, but they're not exported, except for
io.EOF and filepath.SkipDir, neither of which are "proper" errors.

https://codereview.appspot.com/5787068/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

LGTM

https://codereview.appspot.com/5787068/diff/5/runserver.go
File runserver.go (right):

https://codereview.appspot.com/5787068/diff/5/runserver.go#newcode20
runserver.go:20: var ErrNotRunning = errors.New("process not running")
On 2012/03/13 11:27:14, rog wrote:
> it's conventional. try a recursive grep in $GOROOT for
 '^var.*errors\.New'

Just to be fair with William, this wasn't a well established convention
until very recently.

https://codereview.appspot.com/5787068/diff/5/server_test.go
File server_test.go (right):

https://codereview.appspot.com/5787068/diff/5/server_test.go#newcode207
server_test.go:207: dir, err := ioutil.TempDir("", "zktest-")
dir := c.MkDir()

https://codereview.appspot.com/5787068/

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

*** Submitted:

zookeeper: allow CreateServer to work in an existing directory.

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.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/5787068

https://codereview.appspot.com/5787068/

lp:~rogpeppe/gozk/allow-existing-server-dir updated
35. By Roger Peppe

use c.MkDir rather than ioutil.TempDir

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'runserver.go'
2--- runserver.go 2012-02-22 18:56:23 +0000
3+++ runserver.go 2012-03-09 15:19:37 +0000
4@@ -15,17 +15,19 @@
5 "time"
6 )
7
8-var NotRunning = errors.New("process not running")
9+// ErrNotRunning is the error returned when Process cannot
10+// find the currently running zookeeper process.
11+var ErrNotRunning = errors.New("process not running")
12
13 // Process returns a Process referring to the running server from
14 // where it's been stored in pid.txt. If the file does not
15 // exist, or it cannot find the process, it returns the error
16-// NotRunning.
17+// ErrNotRunning.
18 func (srv *Server) Process() (*os.Process, error) {
19 data, err := ioutil.ReadFile(srv.path("pid.txt"))
20 if err != nil {
21 if os.IsNotExist(err) {
22- err = NotRunning
23+ err = ErrNotRunning
24 }
25 return nil, err
26 }
27@@ -39,7 +41,7 @@
28 // getProcess gets a Process from a pid and checks that the
29 // process is actually running. If the process
30 // is not running, then getProcess returns a nil
31-// Process and the error NotRunning.
32+// Process and the error ErrNotRunning.
33 func getProcess(pid int) (*os.Process, error) {
34 p, err := os.FindProcess(pid)
35 if err != nil {
36@@ -53,7 +55,7 @@
37 return p, nil
38 }
39 if err == syscall.ESRCH {
40- return nil, NotRunning
41+ return nil, ErrNotRunning
42 }
43 return nil, errors.New("server running but inaccessible")
44 }
45@@ -65,7 +67,7 @@
46 return err
47 }
48 p, err := srv.Process()
49- if err == nil || err != NotRunning {
50+ if err == nil || err != ErrNotRunning {
51 if p != nil {
52 p.Release()
53 }
54@@ -122,7 +124,7 @@
55 func (srv *Server) Stop() error {
56 p, err := srv.Process()
57 if p == nil {
58- if err != nil {
59+ if err != nil && err != ErrNotRunning {
60 return fmt.Errorf("cannot read process ID of server: %v", err)
61 }
62 return nil
63
64=== modified file 'server.go'
65--- server.go 2011-12-05 19:15:00 +0000
66+++ server.go 2012-03-09 15:19:37 +0000
67@@ -18,9 +18,10 @@
68 zkDir string
69 }
70
71-// CreateServer creates the directory runDir and sets up a ZooKeeper server
72-// environment inside it. It is an error if runDir already exists.
73-// The server will listen on the specified TCP port.
74+// CreateServer creates the directory runDir and sets up a ZooKeeper
75+// server environment inside it. It is an error if runDir already
76+// exists and is not empty. The server will listen on the specified TCP
77+// port.
78 //
79 // The ZooKeeper installation directory is specified by zkDir.
80 // If this is empty, a system default will be used.
81@@ -28,7 +29,16 @@
82 // CreateServer does not start the server.
83 func CreateServer(port int, runDir, zkDir string) (*Server, error) {
84 if err := os.Mkdir(runDir, 0777); err != nil {
85- return nil, err
86+ if !os.IsExist(err) {
87+ return nil, err
88+ }
89+ info, err := ioutil.ReadDir(runDir)
90+ if err != nil {
91+ return nil, err
92+ }
93+ if len(info) != 0 {
94+ return nil, fmt.Errorf("server directory %q is not empty")
95+ }
96 }
97 srv := &Server{runDir: runDir, zkDir: zkDir}
98 if err := srv.writeLog4JConfig(); err != nil {
99@@ -88,6 +98,16 @@
100 panic("not reached")
101 }
102
103+// Addr returns a local host address that can be used
104+// to contact the server when it is running.
105+func (srv *Server) Addr() (string, error) {
106+ port, err := srv.networkPort()
107+ if err != nil {
108+ return "", err
109+ }
110+ return fmt.Sprintf("127.0.0.1:%d", port), nil
111+}
112+
113 // command returns the command used to start the
114 // ZooKeeper server.
115 func (srv *Server) command() ([]string, error) {
116
117=== renamed file 'reattach_test.go' => 'server_test.go'
118--- reattach_test.go 2012-02-10 13:31:40 +0000
119+++ server_test.go 2012-03-09 15:19:37 +0000
120@@ -4,6 +4,7 @@
121 "bufio"
122 "flag"
123 "fmt"
124+ "io/ioutil"
125 . "launchpad.net/gocheck"
126 zk "launchpad.net/gozk/zookeeper"
127 "os"
128@@ -201,3 +202,47 @@
129 err = srv.Stop()
130 c.Assert(err, IsNil)
131 }
132+
133+func (s *S) TestCreateServer(c *C) {
134+ dir, err := ioutil.TempDir("", "zktest-")
135+ c.Assert(err, IsNil)
136+
137+ zkdir := dir + "/zk"
138+ // Check that it creates the new directory.
139+ srv, err := zk.CreateServer(9999, zkdir, "")
140+ c.Assert(err, IsNil)
141+ c.Assert(srv, NotNil)
142+
143+ info, err := os.Stat(zkdir)
144+ c.Assert(err, IsNil)
145+ c.Assert(info.IsDir(), Equals, true)
146+
147+ addr, err := srv.Addr()
148+ c.Assert(err, IsNil)
149+ c.Assert(addr, Equals, "127.0.0.1:9999")
150+
151+ // Check that it fails when called again on the non-empty directory.
152+ _, err = zk.CreateServer(9999, zkdir, "")
153+ c.Assert(err, ErrorMatches, `server directory .* is not empty`)
154+
155+ // Check that Destroy removes the directory.
156+ err = srv.Destroy()
157+ c.Assert(err, IsNil)
158+
159+ _, err = os.Stat(zkdir)
160+ if !os.IsNotExist(err) {
161+ c.Errorf("expected not-exists error, got %v", err)
162+ }
163+
164+ // Check that we can call CreateServer on the empty directory
165+ srv, err = zk.CreateServer(8888, dir, "")
166+ c.Assert(err, IsNil)
167+ c.Assert(srv, NotNil)
168+
169+ addr, err = srv.Addr()
170+ c.Assert(err, IsNil)
171+ c.Assert(addr, Equals, "127.0.0.1:8888")
172+
173+ err = srv.Destroy()
174+ c.Assert(err, IsNil)
175+}

Subscribers

People subscribed via source and target branches