On 23 September 2011 18:21, Gustavo Niemeyer <email address hidden> wrote:
> Review: Needs Fixing
>
> This is looking fantastic. Thanks Roger.
>
> Some details to sort out:
>
>
> [1]
>
> === renamed file 'gozk.go' => 'zookeeper/gozk.go'
>
> Please keep the files at the root of the branch. We'll put it in a
> new series in launchpad so that the gozk/zookeeper reference is
> correct.

ok. i moved the example file into an "example" directory
so that goinstall won't get confused.

>
>
> [2]
>
> +// Conn represents a connection to a set of Zookeeper nodes.
>
> The project name is "ZooKeeper". Please use this casing consistently
> across the code base.

done. + a few original discrepancies in this respect.

> [3]
>
> +// ClientId represents an established ZooKeeper session.  It can be
> +// passed into New to reestablish a connection to an existing session.
>
> There's no New function.

i'd already fixed this.

>
> +func (zk *Conn) RetryChange(path string, flags int, acl []ACL, changeFunc ChangeFunc) os.Error {
>
> Very nice changes in RetryChange too, thanks!

good. worth the bug, i'd say!

> [6]
>
> +       path, err := zk.Create("/test-", "bababum", zookeeper.SEQUENCE|zookeeper.EPHEMERAL, zookeeper.WorldACL(zookeeper.PERM_ALL))
>
> These lines make me unhappy about the length of "zookeeper". I'm thinking we
> might well rename it to "zk" in a follow up branch, but please keep it like
> this for now. Just for comparison:
>
> path, err := conn.Create("/test-", "bababum", zk.SEQUENCE|zk.EPHEMERAL, zk.WorldACL(zk.PERM_ALL))

my inclination would be to keep the name as "zookeeper", but
people can import as zk if they want. then strangers to code that's
using it will see "launchpad.net/zookeeper" and know instantly
what the package is about.

also, people can easily define constants for this kind of thing if they wish.

> +       c.Assert(stat, Equals, (*zookeeper.Stat)(nil))
>
> c.Assert(stat, IsNil)

done.

> [8]
>
> +// Server sets up a zookeeper server environment inside dataDir
>
> ZooKeeper in this file as well.

done.

> [9]
>
> +// for a server that listening on the specified TCP port, sending
>
> s/listening/listens/

done

> [10]
>
> +// Server does not actually start the server. Instead it returns
> +// a command line, suitable for passing to exec.Command,
> +// for example.
>
> Let's do a bit better than this. There's no reason to keep the
> responsibility of creating the directory, starting, stopping,
> cleaning, etc, with the caller. We need this for real, not just
> due to testing, but in juju for the local deployment case.

i thought quite a bit about this.
the problem with doing the starting ourselves is that
there are many things the caller might wish to do
with the running executable. for example:
- logging (see the custom logging in suite_test.go)
- running as a different user
- tweaking the config file before starting
- stashing the pid for long-term killability
- starting the same server several times on the same
  directory (as is done in this test suite). ok, your proposal
  does deal with this.

-
>
> I suggest we define a Server type as:
>
>  type Server struct {
>      runDir, installDir string
>      cmd *exec.Cmd
>  }
>
> and:
>
>  func zookeeper.NewServer(runDir) *Server
>
> Then, we should have this as well:
>
>  func (s *Server) Create(installDir string) os.Error
>  func (s *Server) Start() os.Error
>  func (s *Server) Stop() os.Error
>  func (s *Server) Destroy() os.Error
>
> Please push this in a spearate merge proposal. I'm happy to
> merge the current version as is for the moment.
>
>
> [11]
>
> +var log4jProperties = []byte(`
>
> No need to have two copies of that file in memory at all times. Use a const
> and []byte() it when necessary.

i thought about this. the compiler could easily lose the const copy,
as it's only referred to once, in which case you'd get the
best of both worlds, but i don't know if it does or not.

done.

> [12]
>
> +       classPath, _ := filepath.Glob(filepath.Join(dir, "zookeeper-*.jar"))
> +       more, _ := filepath.Glob(filepath.Join(dir, "lib/*.jar"))
>
> Please check the errors.

the docs say that the only error that glob can return is if the
pattern is malformed.
those patterns are evidently not.

> [13]
>
> +       fd, err := os.Open(zookeeperEnviron)
>
> That's a file, or an f, not an fd.

done

> [14]
>
> +               line, err := r.ReadSlice('\n')
>
> ReadLine()

i started off using ReadLine and then switched to ReadSlice
to avoid the complication of lines that were too long.
doing the isPrefix dance right isn't worth it to save a few
allocations.

> [15]
>
> +               // strip quotes
> +               if path[0] == '"' {
> +                       path = path[1 : len(path)-1]
> +               }
>
> Please use Trim.  This logic assumes it knows the position of quotes.

done

PTAL