state: add WatchMachines

state: add WatchMachines



https://code.launchpad.net/~dave-cheney/juju/go-machine-state-watcher/+merge/105101

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-05-08 18:23:04]

Please take a look.

[fwereade, 2012-05-08 18:37:45]

LGTM modulo possible naming tweak.

https://codereview.appspot.com/6202060/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6202060/diff/2001/state/watcher.go#newcode297
state/watcher.go:297: type MachineWatcher struct {
I *think* this is more a MachinesWatcher than a MachineWatcher.

[rog, 2012-05-08 18:47:33]

LGTM

https://codereview.appspot.com/6202060/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6202060/diff/2001/state/watcher.go#newcode297
state/watcher.go:297: type MachineWatcher struct {
On 2012/05/08 18:37:45, fwereade wrote:
> I *think* this is more a MachinesWatcher than a MachineWatcher.

+1

https://codereview.appspot.com/6202060/diff/2001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6202060/diff/2001/state/watcher_test.go#newcod...
state/watcher_test.go:219: {func(s *state.State) error { _, err :=
s.AddMachine(); return err }, watcher.ChildrenChange{Added:
[]string{"machine-0000000000"}}},
i think that putting these all on one line is stretching things a bit.
i think i'd probably put the test function on a separate line from the change
list.

[dave_cheney.net, 2012-05-08 18:49:38]

Please take a look.

[dave_cheney.net, 2012-05-08 18:54:22]

Please take a look.

https://codereview.appspot.com/6202060/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6202060/diff/2001/state/watcher.go#newcode297
state/watcher.go:297: type MachineWatcher struct {
On 2012/05/08 18:37:45, fwereade wrote:
> I *think* this is more a MachinesWatcher than a MachineWatcher.

Done.

https://codereview.appspot.com/6202060/diff/2001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6202060/diff/2001/state/watcher_test.go#newcod...
state/watcher_test.go:219: {func(s *state.State) error { _, err :=
s.AddMachine(); return err }, watcher.ChildrenChange{Added:
[]string{"machine-0000000000"}}},
PTAL. gofmt gives me limited scope here.

[TheMue, 2012-05-08 18:54:23]

LGTM, only one little comment.

https://codereview.appspot.com/6202060/diff/1005/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6202060/diff/1005/state/state.go#newcode62
state/state.go:62: return newMachinesWatcher(s, "/machines")
Typically the path in ZooKeeper is returned by a method. Here it would be
zkMachines(). To keep it constant I would change it here too.

[dave_cheney.net, 2012-05-08 18:59:37]

https://codereview.appspot.com/6202060/diff/1005/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6202060/diff/1005/state/state.go#newcode62
state/state.go:62: return newMachinesWatcher(s, "/machines")
A method, or a constant ? I couldn't find any pattern here to follow, the code
directly above this just does string munging.

Having said that, it is important that this is a known constant, because
watching '/machines/' does not work, at all.

[dave_cheney.net, 2012-05-08 19:03:21]

Please take a look.

[dave_cheney.net, 2012-05-08 19:05:37]

Please take a look.

[dave_cheney.net, 2012-05-08 19:31:12]

Please take a look.

[fwereade, 2012-05-08 19:37:40]

LGTM

[TheMue, 2012-05-08 19:39:15]

One little last comment.

https://codereview.appspot.com/6202060/diff/13001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6202060/diff/13001/state/machine.go#newcode63
state/machine.go:63: func zkMachinesPath() string { return "/machines" }
The comment is missing.

// zkMachinesPath returns the ZooKeeper parent path for all machines.

[dave_cheney.net, 2012-05-08 19:42:35]

Please take a look.

https://codereview.appspot.com/6202060/diff/13001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6202060/diff/13001/state/machine.go#newcode63
state/machine.go:63: func zkMachinesPath() string { return "/machines" }
Done. Sorry that got eaten along the way.

[TheMue, 2012-05-08 19:43:59]

LGTM

[niemeyer, 2012-05-09 17:26:34]

Woohay first branch! :)

https://codereview.appspot.com/6202060/diff/11002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6202060/diff/11002/state/machine.go#newcode64
state/machine.go:64: func zkMachinesPath() string { return "/machines" }
Why isn't this a const?

https://codereview.appspot.com/6202060/diff/11002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6202060/diff/11002/state/watcher.go#newcode296
state/watcher.go:296: // MachinesWatcher observes changes children of the
/machines key.
.. in/to children?

https://codereview.appspot.com/6202060/diff/11002/state/watcher.go#newcode302
state/watcher.go:302: Changes chan watcher.ChildrenChange
Changes is a method on every other watcher, I believe. If there's no need for it
to be a method, I'm happy to have it changed, but we should have all of them one
way or the other.

https://codereview.appspot.com/6202060/diff/11002/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6202060/diff/11002/state/watcher_test.go#newco...
state/watcher_test.go:220: watcher.ChildrenChange{Added:
[]string{"machine-0000000000"}},
It feels like the indentation could be a bit more readable here. For example:

    {
        func(...) ... {
            ...
        },
        watcher.ChildrenChange{Added: ...},
    },

or something along these lines.

[dave_cheney.net, 2012-05-09 17:50:24]

Please take a look.

https://codereview.appspot.com/6202060/diff/11002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6202060/diff/11002/state/machine.go#newcode64
state/machine.go:64: func zkMachinesPath() string { return "/machines" }
On 2012/05/09 17:26:35, niemeyer wrote:
> Why isn't this a const?

Done.

https://codereview.appspot.com/6202060/diff/11002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6202060/diff/11002/state/watcher.go#newcode296
state/watcher.go:296: // MachinesWatcher observes changes children of the
/machines key.
On 2012/05/09 17:26:35, niemeyer wrote:
> .. in/to children?

Done.

https://codereview.appspot.com/6202060/diff/11002/state/watcher.go#newcode302
state/watcher.go:302: Changes chan watcher.ChildrenChange
On 2012/05/09 17:26:35, niemeyer wrote:
> Changes is a method on every other watcher, I believe. If there's no need for
it
> to be a method, I'm happy to have it changed, but we should have all of them
one
> way or the other.

Done.

https://codereview.appspot.com/6202060/diff/11002/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6202060/diff/11002/state/watcher_test.go#newco...
state/watcher_test.go:220: watcher.ChildrenChange{Added:
[]string{"machine-0000000000"}},
On 2012/05/09 17:26:35, niemeyer wrote:
> It feels like the indentation could be a bit more readable here. For example:
> 
>     {
>         func(...) ... {
>             ...
>         },
>         watcher.ChildrenChange{Added: ...},
>     },
> 
> or something along these lines.

Done.

[niemeyer, 2012-05-09 18:41:00]

Thanks. LGTM!

[dave_cheney.net, 2012-05-09 19:16:36]

*** Submitted:

state: add WatchMachines

R=fwereade, rog, TheMue, niemeyer
CC=
https://codereview.appspot.com/6202060