More watchers for Unit.

More watchers for Unit.

Adding of NeedsUpgradeWatcher, ResolvedWatcher and PortsWatcher
for Units. They all are based on the new ExistenceWatcher for
the creation, content changing and deletion of nodes.

https://code.launchpad.net/~themue/juju/go-state-more-unit-watchers/+merge/101760

(do not edit description out of merge proposal)

[TheMue, 2012-04-12 15:37:11]

Please take a look.

[rog, 2012-04-16 11:43:01]

looks good.

https://codereview.appspot.com/6011047/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode489
state/unit.go:489: // NeedsUpgradeWatcher observes changes to a upgrade flag
node.
 // NeedsUpgradeWatcher observes changes to a unit's upgrade flag.
?

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode531
state/unit.go:531: defer close(w.changeChan)
is this how callers are supposed to know that the upgrade watcher is shutting
down? there's no Dying method, so i guess so. if so, it should be documented
(it's different from the ExistenceWatcher way); if not then you should a a Dying
method and ensure that w.tomb is in a dying state before closing changeChan.

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode614
state/unit.go:614: // PortsWatcher observes changes to a ports node.
// PortsWatcher observes changes to a unit's open ports.

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode656
state/unit.go:656: defer close(w.changeChan)
same applies here as to NeedsUpgradeWatcher.

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode663
state/unit.go:663: ports := &struct{ Open []Port }{}
var ports struct {
    Open []Port
}
would be clearer i think (and would avoid the unnecessary double indirection).

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go#newcode13
state/watch_test.go:13: return change, ok, false
you could probably just Assert(ok, Equals, true) here
and thus simplify the interface to receiveChange.
That applies to most of the similar tests below - those that don't test for the
changes channel being closed can factor out the closed test.

another possibility is to pass expected values for timeout and closed into the
function:

receiveChange := func(w *state.ConfigWatcher, expectClosed, expectTimeout bool)
*state.ConfigNode {
    etc
}

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go#newcode14
state/watch_test.go:14: case <-time.After(time.Second):
i think a second is too long here. 100 * time.Millsecond?
same applies below, let's not make testing take too long.

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go#newcode10
state/watcher/watcher.go:10: // of a node and, if it's true, its content.
// ExistenceChange holds information on the existence
// and contents of a node. Content will be empty when the
// node does not exist.
?

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go#newcode17
state/watcher/watcher.go:17: // notification (including the content) when it is
created, changed
s/ (including the content)//

it's implied by the rest of the sentence and the ExistenceChange type.

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go#newcode94
state/watcher/watcher.go:94: content, _, watch, err := w.zk.GetW(w.path)
it seems slightly odd that when a node gets deleted, we ignore what we've just
been told and call GetW and then ExistsW on the node anyway.
i appreciate that passing the event type into update might complicate the logic
a little, but i think it might be worth doing. something like this perhaps?

func (w *ExistenceWatcher) update(event int) (nextWatch <-chan zookeeper.Event,
err error
     ) {
    var content string
    var watch <-chan zookeeper.Event
    if event != zookeeper.EVENT_DELETED {
        content, _, watch, err = w.zk.GetW(w.path)
    }
    switch {
    case event == zookeeper.EVENT_DELETED || zookeeper.IsError(err,
zookeeper.ZNONODE):
         call existsW
    etc
}

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher_test.go
File state/watcher/watcher_test.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher_test.go#n...
state/watcher/watcher_test.go:50: receiveChange := func(w
*watcher.ExistenceWatcher) (*watcher.ExistenceChange, bool, bool) {
same comment applies here as in state/watch_test.go.

[TheMue, 2012-04-16 15:45:30]

Please take a look.

https://codereview.appspot.com/6011047/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode489
state/unit.go:489: // NeedsUpgradeWatcher observes changes to a upgrade flag
node.
On 2012/04/16 11:43:01, rog wrote:
>  // NeedsUpgradeWatcher observes changes to a unit's upgrade flag.
> ?

Done.

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode531
state/unit.go:531: defer close(w.changeChan)
On 2012/04/16 11:43:01, rog wrote:
> is this how callers are supposed to know that the upgrade watcher is shutting
> down? there's no Dying method, so i guess so. if so, it should be documented
> (it's different from the ExistenceWatcher way); if not then you should a a
Dying
> method and ensure that w.tomb is in a dying state before closing changeChan.

This watcher is different from the ExistenceWatcher as it is just using it. It's
more comparable to the ConfigWatcher in state.go.

But yes, Dying() has been forgotton.

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode614
state/unit.go:614: // PortsWatcher observes changes to a ports node.
On 2012/04/16 11:43:01, rog wrote:
> // PortsWatcher observes changes to a unit's open ports.

Done.

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode656
state/unit.go:656: defer close(w.changeChan)
On 2012/04/16 11:43:01, rog wrote:
> same applies here as to NeedsUpgradeWatcher.

Done.

https://codereview.appspot.com/6011047/diff/1/state/unit.go#newcode663
state/unit.go:663: ports := &struct{ Open []Port }{}
On 2012/04/16 11:43:01, rog wrote:
> var ports struct {
>     Open []Port
> }
> would be clearer i think (and would avoid the unnecessary double indirection).

Done. Looks indeed better.

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go#newcode13
state/watch_test.go:13: return change, ok, false
On 2012/04/16 11:43:01, rog wrote:
> you could probably just Assert(ok, Equals, true) here
> and thus simplify the interface to receiveChange.
> That applies to most of the similar tests below - those that don't test for
the
> changes channel being closed can factor out the closed test.
> 
> another possibility is to pass expected values for timeout and closed into the
> function:
> 
> receiveChange := func(w *state.ConfigWatcher, expectClosed, expectTimeout
bool)
> *state.ConfigNode {
>     etc
> }

Thought about it too. But then I decided for a clean seperation of the simple
retrievement with a possible timeout (had some due to errors while development)
and the assertions in the test. To me this way is more readable, even if the
code is bit more bloated.

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go#newcode14
state/watch_test.go:14: case <-time.After(time.Second):
On 2012/04/16 11:43:01, rog wrote:
> i think a second is too long here. 100 * time.Millsecond?
> same applies below, let's not make testing take too long.

Done. And all other helper timeouts too.

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go#newcode10
state/watcher/watcher.go:10: // of a node and, if it's true, its content.
On 2012/04/16 11:43:01, rog wrote:
> // ExistenceChange holds information on the existence
> // and contents of a node. Content will be empty when the
> // node does not exist.
> ?

Done. Thx, help of a native speaker is appreciated.

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go#newcode17
state/watcher/watcher.go:17: // notification (including the content) when it is
created, changed
On 2012/04/16 11:43:01, rog wrote:
> s/ (including the content)//
> 
> it's implied by the rest of the sentence and the ExistenceChange type.

Done.

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go#newcode94
state/watcher/watcher.go:94: content, _, watch, err := w.zk.GetW(w.path)
On 2012/04/16 11:43:01, rog wrote:
> it seems slightly odd that when a node gets deleted, we ignore what we've just
> been told and call GetW and then ExistsW on the node anyway.
> i appreciate that passing the event type into update might complicate the
logic
> a little, but i think it might be worth doing. something like this perhaps?
> 
> func (w *ExistenceWatcher) update(event int) (nextWatch <-chan
zookeeper.Event,
> err error
>      ) {
>     var content string
>     var watch <-chan zookeeper.Event
>     if event != zookeeper.EVENT_DELETED {
>         content, _, watch, err = w.zk.GetW(w.path)
>     }
>     switch {
>     case event == zookeeper.EVENT_DELETED || zookeeper.IsError(err,
> zookeeper.ZNONODE):
>          call existsW
>     etc
> }

Hmm, ok, which event would you pass for the first call of update()? At this
moment you're unsure if it exists. And where does err in switch come from?

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher_test.go
File state/watcher/watcher_test.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher_test.go#n...
state/watcher/watcher_test.go:50: receiveChange := func(w
*watcher.ExistenceWatcher) (*watcher.ExistenceChange, bool, bool) {
On 2012/04/16 11:43:01, rog wrote:
> same comment applies here as in state/watch_test.go.

Same arguments to keep it here. Adjusted timeout.

[rog, 2012-04-16 16:02:50]

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watch_test.go#newcode13
state/watch_test.go:13: return change, ok, false
On 2012/04/16 15:45:31, TheMue wrote:
> On 2012/04/16 11:43:01, rog wrote:
> > you could probably just Assert(ok, Equals, true) here
> > and thus simplify the interface to receiveChange.
> > That applies to most of the similar tests below - those that don't test for
> the
> > changes channel being closed can factor out the closed test.
> > 
> > another possibility is to pass expected values for timeout and closed into
the
> > function:
> > 
> > receiveChange := func(w *state.ConfigWatcher, expectClosed, expectTimeout
> bool)
> > *state.ConfigNode {
> >     etc
> > }
> 
> Thought about it too. But then I decided for a clean seperation of the simple
> retrievement with a possible timeout (had some due to errors while
development)
> and the assertions in the test. To me this way is more readable, even if the
> code is bit more bloated.

fair enough.

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6011047/diff/1/state/watcher/watcher.go#newcode94
state/watcher/watcher.go:94: content, _, watch, err := w.zk.GetW(w.path)
On 2012/04/16 15:45:31, TheMue wrote:
> On 2012/04/16 11:43:01, rog wrote:
> > it seems slightly odd that when a node gets deleted, we ignore what we've
just
> > been told and call GetW and then ExistsW on the node anyway.
> > i appreciate that passing the event type into update might complicate the
> logic
> > a little, but i think it might be worth doing. something like this perhaps?
> > 
> > func (w *ExistenceWatcher) update(event int) (nextWatch <-chan
> zookeeper.Event,
> > err error
> >      ) {
> >     var content string
> >     var watch <-chan zookeeper.Event
> >     if event != zookeeper.EVENT_DELETED {
> >         content, _, watch, err = w.zk.GetW(w.path)
> >     }
> >     switch {
> >     case event == zookeeper.EVENT_DELETED || zookeeper.IsError(err,
> > zookeeper.ZNONODE):
> >          call existsW
> >     etc
> > }
> 
> Hmm, ok, which event would you pass for the first call of update()? At this
> moment you're unsure if it exists. And where does err in switch come from?

you could pass any event that's not EVENT_DELETED - EVENT_CHANGED would be fine.

err is defined in the same place it is currently - as a return parameter.

[niemeyer, 2012-04-16 16:58:46]

As we discussed over IRC, can you please break that down into smaller chunks?

https://codereview.appspot.com/6011047/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6011047/diff/6001/state/state.go#newcode273
state/state.go:273: func (w *ConfigWatcher) Dying() <-chan struct{} {
Why is this necessary?

https://codereview.appspot.com/6011047/diff/6001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6011047/diff/6001/state/unit.go#newcode316
state/unit.go:316: // WatchResolved creates a watcher for the resolved node of
the unit.
There's no such thing as a "resolved node" documented in this API. We need to
document these methods in terms of what they mean semantically for juju, rather
than what they mean for the interaction of these methods with zookeeper. For
example:

// WatchResolved returns a watcher that fires when the unit
// is marked as having had its problems resolved. See
// SetResolved for details.

Please review the documentation for the other methods.

https://codereview.appspot.com/6011047/diff/6001/state/unit.go#newcode480
state/unit.go:480: if none && mode == ResolvedNone {
none? Does it mean acceptNone? Please fix the argument name and document it.

[niemeyer, 2012-04-16 17:35:00]

More notes on the changes in watcher to get you going tomorrow.

https://codereview.appspot.com/6011047/diff/6001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6011047/diff/6001/state/watcher/watcher.go#new...
state/watcher/watcher.go:12: type ExistenceChange struct {
This isn't watching just existence.. it's watching both existence and contents.
How's that different from the ContentsWatcher, and why do we need both? Any
higher-level watcher that needs contents may be implemented on top of a watcher
that offers existence *and* contents. I suggest dropping this type, and
modifying ContentsWatcher to handle both problems.

This should be done in an independent branch.

https://codereview.appspot.com/6011047/diff/6001/state/watcher/watcher.go#new...
state/watcher/watcher.go:96: case err != nil && !zookeeper.IsError(err,
zookeeper.ZNONODE):
This check should be inverted. We care about the case ZNONODE, so let's be
explicit about it:

case zookeeper.IsError(err, zookeeper.ZNONODE):
   ...

https://codereview.appspot.com/6011047/diff/6001/state/watcher/watcher.go#new...
state/watcher/watcher.go:100: _, watch, err = w.zk.ExistsW(w.path)
This is bogus. It is assuming that the remote state of the node is known here,
but it's not. The current state of the node is in that parameter that is being
ignored.

The overall logic must be fixed to take that into consideration.

[niemeyer, 2012-04-18 19:16:28]

For the record, this work is being split down into other smaller branches.
Please do keep these reviews in mind, though.