Issue 5905064: Watcher package as base for all state watcher. (Closed)
DescriptionWatcher package as base for all state watcher.
Like the presence package observing the presence of a ZooKeeper
node the Watcher can be configured to observe content or children
changes of nodes. Those changes are continuously delivered until
the Watcher is stopped.
https://code.launchpad.net/~themue/juju/go-state-watcher/+merge/99330
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 18
Patch Set 2 : Watcher package as base for all state watcher. #
Total comments: 24
Patch Set 3 : Watcher package as base for all state watcher. #Patch Set 4 : Watcher package as base for all state watcher. #
Total comments: 34
Patch Set 5 : Watcher package as base for all state watcher. #
Total comments: 24
Patch Set 6 : Watcher package as base for all state watcher. #Patch Set 7 : Watcher package as base for all state watcher. #
Total comments: 4
Patch Set 8 : Watcher package as base for all state watcher. #
Total comments: 8
Patch Set 9 : Watcher package as base for all state watcher. #
MessagesTotal messages: 24
Please take a look.
Sign in to reply to this message.
Going in a good direction. Main thing is to sort out the watcher type distinction, as detailed below. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode1 state/watcher/watcher.go:1: // launchpad.net/juju/state/watcher Please drop this. We should add proper licensing headers to all files, but let's tackle this all at once later. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode3 state/watcher/watcher.go:3: // Copyright (c) 2011-2012 Canonical Ltd. 2012 only. 2011 is no more. ;) https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode47 state/watcher/watcher.go:47: // containing the node content as first element or its This API isn't great. There are two different types of watchers that have different interfaces and are being built with a single function for the convenience of the implementation rather than the user. As a side effect, the user will have to know why there's a list with a single element on all uses of that interface. Let's split that up into ContentWatcher and ChildrenWatcher and offer an API that is relevant for each of them. The implementation here should have almost no overhead, as you can continue to share most of the implementation internally. Just rename Watcher to watcher, and add a second channel on it, and implement ContentWatcher and ChildrenWatcher embedding the watcher and using the relevant channel on each. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode71 state/watcher/watcher.go:71: w.tomb.Killf("watcher: ZooKeeper critical session event: %v", evt) s/ZooKeeper //; the error will come decorated already. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode99 state/watcher/watcher.go:99: // watchChildren retrieves the children of a node and returns the next What's the meaning of the result? Please use named results and also mention that in the documentation. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:112: // of a change the new data will be sent to the receiver. Ditto. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:114: if reflect.DeepEqual(buffer, w.buffer) { This should be withing each of the watch functions, since it's a different implementation for each. It also feels like it's abusing a bit the reflect package. The types for these two lists is fixed, and it's trivial to iterate over it to check for equality. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:117: firstContentCall := w.changeType == ContentChange && w.buffer == nil Again, this is calling a common function from a type-specific function so that the common function has to do type-specific logic. The type-specific behavior should be within the type-specific function. I suspect the common update() will actually lose its sense if we have different channel types for each type of watcher. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:130: // singleEventChan creates the initial event channel and Please inline this where it's being called. It's very specific logic that makes no sense out of the context it's being used.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode1 state/watcher/watcher.go:1: // launchpad.net/juju/state/watcher On 2012/03/26 21:06:38, niemeyer wrote: > Please drop this. We should add proper licensing headers to all files, but let's > tackle this all at once later. Done. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode3 state/watcher/watcher.go:3: // Copyright (c) 2011-2012 Canonical Ltd. On 2012/03/26 21:06:38, niemeyer wrote: > 2012 only. 2011 is no more. ;) Done.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode47 state/watcher/watcher.go:47: // containing the node content as first element or its On 2012/03/26 21:06:38, niemeyer wrote: > This API isn't great. There are two different types of watchers that have > different interfaces and are being built with a single function for the > convenience of the implementation rather than the user. As a side effect, the > user will have to know why there's a list with a single element on all uses of > that interface. > > Let's split that up into ContentWatcher and ChildrenWatcher and offer an API > that is relevant for each of them. The implementation here should have almost no > overhead, as you can continue to share most of the implementation internally. > Just rename Watcher to watcher, and add a second channel on it, and implement > ContentWatcher and ChildrenWatcher embedding the watcher and using the relevant > channel on each. Done. Followed an approach discussed with Roger. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode71 state/watcher/watcher.go:71: w.tomb.Killf("watcher: ZooKeeper critical session event: %v", evt) On 2012/03/26 21:06:38, niemeyer wrote: > s/ZooKeeper //; the error will come decorated already. Done. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcode99 state/watcher/watcher.go:99: // watchChildren retrieves the children of a node and returns the next On 2012/03/26 21:06:38, niemeyer wrote: > What's the meaning of the result? Please use named results and also mention that > in the documentation. Done. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:112: // of a change the new data will be sent to the receiver. On 2012/03/26 21:06:38, niemeyer wrote: > Ditto. Done. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:114: if reflect.DeepEqual(buffer, w.buffer) { On 2012/03/26 21:06:38, niemeyer wrote: > This should be withing each of the watch functions, since it's a different > implementation for each. It also feels like it's abusing a bit the reflect > package. The types for these two lists is fixed, and it's trivial to iterate > over it to check for equality. New approach handles it better and without reflect. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:117: firstContentCall := w.changeType == ContentChange && w.buffer == nil On 2012/03/26 21:06:38, niemeyer wrote: > Again, this is calling a common function from a type-specific function so that > the common function has to do type-specific logic. The type-specific behavior > should be within the type-specific function. > > I suspect the common update() will actually lose its sense if we have different > channel types for each type of watcher. Done. https://codereview.appspot.com/5905064/diff/1/state/watcher/watcher.go#newcod... state/watcher/watcher.go:130: // singleEventChan creates the initial event channel and On 2012/03/26 21:06:38, niemeyer wrote: > Please inline this where it's being called. It's very specific logic that makes > no sense out of the context it's being used. Done.
Sign in to reply to this message.
LGTM with a couple of minors. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:49: watch := func() <-chan zookeeper.Event { i think now all the logic has gone into update(), this can be replaced by: watch, cont := w.updater.update() if !cont { return } https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:62: case evt := <-watch: i've just thought of a minor problem with this general approach - the updater might want to do something different on different kinds of events (e.g. EVENT_DELETED). we could solve that by sending the event to the update method (and we'd probably want to add the initial event as an argument to watcher.init too, so the first event was as expected). https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:176: diff := make(map[string]bool) not sure about diff as a name here - it sounds like it holds the difference from the old set of children (mind you, "m" was worse :-]). how about renaming the above "children" variable because it's not much used ("childSlice" ? dunno), and then renaming "diff" to "children"? https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:196: sort.Strings(change.Del) is there any particular reason to sort these?
Sign in to reply to this message.
https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.go File state/watcher/watcher_test.go (right): https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:15: // wrapper shows the returning of user defined types instead of i don't see functionality in the watcher package this tests. content strings are only tested for equality, so we can simply test different strings, without worrying about yaml etc. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:66: s.changeContent(c, "foo") i'd add another s.changeContent(c, "foo") here to check that events are only sent when the content actually changes. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:83: case <-time.After(time.Second): 1s seems like a long time to wait here. 200ms should be ample. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:87: err = watcher.Stop() check that watcher.Changes has been closed here. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:91: func (s *WatcherSuite) TestWrappedContentWatcher(c *C) { i don't think this test adds anything. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:165: c.Assert(change.Del, DeepEquals, []string{"foo"}) it's a pity we can't test multiple changes at once, but i can't think of a way of doing so reliably. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:171: case <-time.After(time.Second): 1s seems like a long time to wait here. 200ms should be ample. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:189: c.Assert(err, IsNil) d
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.go File state/watcher/watcher_test.go (right): https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:15: // wrapper shows the returning of user defined types instead of On 2012/03/27 16:22:24, rog wrote: > i don't see functionality in the watcher package this tests. > content strings are only tested for equality, so we can simply test different > strings, without worrying about yaml etc. Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:66: s.changeContent(c, "foo") On 2012/03/27 16:22:24, rog wrote: > i'd add another s.changeContent(c, "foo") here to check that events are only > sent when the content actually changes. Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:83: case <-time.After(time.Second): On 2012/03/27 16:22:24, rog wrote: > 1s seems like a long time to wait here. 200ms should be ample. Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:87: err = watcher.Stop() On 2012/03/27 16:22:24, rog wrote: > check that watcher.Changes has been closed here. Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:91: func (s *WatcherSuite) TestWrappedContentWatcher(c *C) { On 2012/03/27 16:22:24, rog wrote: > i don't think this test adds anything. Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:165: c.Assert(change.Del, DeepEquals, []string{"foo"}) On 2012/03/27 16:22:24, rog wrote: > it's a pity we can't test multiple changes at once, but i can't think of a way > of doing so reliably. Yip, tried it, but locally events are fired too fast. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:171: case <-time.After(time.Second): On 2012/03/27 16:22:24, rog wrote: > 1s seems like a long time to wait here. 200ms should be ample. Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher_test.g... state/watcher/watcher_test.go:189: c.Assert(err, IsNil) On 2012/03/27 16:22:24, rog wrote: > d Done.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:49: watch := func() <-chan zookeeper.Event { On 2012/03/27 15:09:54, rog wrote: > i think now all the logic has gone into update(), this can be replaced by: > > watch, cont := w.updater.update() > if !cont { > return > } Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:62: case evt := <-watch: On 2012/03/27 15:09:54, rog wrote: > i've just thought of a minor problem with this general approach - the updater > might want to do something different on different kinds of events (e.g. > EVENT_DELETED). > > we could solve that by sending the event to the update method (and we'd probably > want to add the initial event as an argument to watcher.init too, so the first > event was as expected). Done. https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:196: sort.Strings(change.Del) On 2012/03/27 15:09:54, rog wrote: > is there any particular reason to sort these? It's to get better comparable emitted values.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/5001/state/watcher/watcher.go#new... state/watcher/watcher.go:176: diff := make(map[string]bool) On 2012/03/27 15:09:54, rog wrote: > not sure about diff as a name here - it sounds like it holds the difference from > the old set of children (mind you, "m" was worse :-]). > > how about renaming the above "children" variable because it's not much used > ("childSlice" ? dunno), and then renaming "diff" to "children"? Done.
Sign in to reply to this message.
I think this is still going in a positive direction, but unfortunately I'll have to bother you with a few comments. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:11: // updates individually. There's some mix up going on which would be good to get cleaned. The role of updater is unclear, and that is shining through its documentation, that is actually made in terms of watcher, ChildrenWatcher, and ContentWatcher, rather than its own purpose. In fact, these types are all mixed up, without clear boundaries, and if you dig through you'll note that all of that weight comes from an attempt to share about 15 lines of code in the loop method. Please follow on and break both types completely, getting rid of updater and watcher. I'm betting the code will look cleaner, simpler, and thus easier to understand. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:53: // watch := func() <-chan zookeeper.Event { The dead code may be removed. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:80: // and delivers those via the Change() method. // ContentWatcher observes a ZooKeeper node and delivers a // notification when a content change is detected. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:89: // NewContentWatcher creates a new content watcher. This comment may be dropped. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:101: return w, nil If there are no possibilities of error, we don't need an error result. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:105: // returned channel each time it changes. // Changes returns a channel that will receive the new node // content when a change is detected. Note that multiple // changes may be observed as a single event in the channel. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:112: // emits changed content via the changeChan. Needs a new comment, and the comment should not mention the code flow. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:113: func (w *ContentWatcher) update(event zookeeper.Event) (nextWatch <-chan zookeeper.Event, cont bool) { The use of "cont" in those methods feels a bit non-conventional, and seems to encourage bad practices. For example, the logic below returns false if the node is deleted. But.. and then what? The loop stops, returns, the watcher never fires again.. no errors, just silent misbehavior. This should really return (chan, error), and then return nil or the error as appropriate. If a watched node goes away, that's an error that should be returned. The call site, within loop, should be the one doing the "if err != nil { w.tomb.Kill(err); return }". https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:135: // ContentWatcher it just closes the changeChan. Observe: // For the ContentWatcher it just closes the changeChan. vs. func (w *ContentWatcher) close() { close(w.changeChan) } No need to code twice. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:146: New []string s/Del/Removed/ s/New/Added/ And drop comments please. ChildrenChange.Added/Removed is clear. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:150: // children and delivers those via the Change() method. // ChildrenWatcher observes a ZooKeeper node and delivers a // notification when child nodes are added or removed. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:159: // NewWatcher creates a new watcher. Comment is wrong, but may actually be dropped. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:172: return w, nil If there are no errors, no need for an error result. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:176: // the node on the returned channel each time they are changing. // Changes returns a channel that will receive the changes // performed to the set of children of the watched node. // Note that multiple changes may be observed as a single // event in the channel. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:183: // are added or deleted and emits these changes via the changeChan. Needs a new comment, and the comment should not mention the code flow. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:184: func (w *ChildrenWatcher) update(event zookeeper.Event) (nextWatch <-chan zookeeper.Event, cont bool) { Please return error rather than bool, and fix appropriately. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:224: // ChildrenWatcher it just closes the changeChan. Comment needs love.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:11: // updates individually. On 2012/03/28 00:28:59, niemeyer wrote: > There's some mix up going on which would be good to get cleaned. The role of > updater is unclear, and that is shining through its documentation, that is > actually made in terms of watcher, ChildrenWatcher, and ContentWatcher, rather > than its own purpose. In fact, these types are all mixed up, without clear > boundaries, and if you dig through you'll note that all of that weight comes > from an attempt to share about 15 lines of code in the loop method. > > Please follow on and break both types completely, getting rid of updater and > watcher. I'm betting the code will look cleaner, simpler, and thus easier to > understand. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:53: // watch := func() <-chan zookeeper.Event { On 2012/03/28 00:28:59, niemeyer wrote: > The dead code may be removed. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:80: // and delivers those via the Change() method. On 2012/03/28 00:28:59, niemeyer wrote: > // ContentWatcher observes a ZooKeeper node and delivers a > // notification when a content change is detected. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:89: // NewContentWatcher creates a new content watcher. On 2012/03/28 00:28:59, niemeyer wrote: > This comment may be dropped. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:101: return w, nil On 2012/03/28 00:28:59, niemeyer wrote: > If there are no possibilities of error, we don't need an error result. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:105: // returned channel each time it changes. On 2012/03/28 00:28:59, niemeyer wrote: > // Changes returns a channel that will receive the new node > // content when a change is detected. Note that multiple > // changes may be observed as a single event in the channel. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:112: // emits changed content via the changeChan. On 2012/03/28 00:28:59, niemeyer wrote: > Needs a new comment, and the comment should not mention the code flow. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:113: func (w *ContentWatcher) update(event zookeeper.Event) (nextWatch <-chan zookeeper.Event, cont bool) { On 2012/03/28 00:28:59, niemeyer wrote: > The use of "cont" in those methods feels a bit non-conventional, and seems to > encourage bad practices. For example, the logic below returns false if the node > is deleted. But.. and then what? The loop stops, returns, the watcher never > fires again.. no errors, just silent misbehavior. > > This should really return (chan, error), and then return nil or the error as > appropriate. If a watched node goes away, that's an error that should be > returned. The call site, within loop, should be the one doing the "if err != nil > { w.tomb.Kill(err); return }". Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:135: // ContentWatcher it just closes the changeChan. On 2012/03/28 00:28:59, niemeyer wrote: > Observe: > > // For the ContentWatcher it just closes the changeChan. > > vs. > > func (w *ContentWatcher) close() { close(w.changeChan) } > > No need to code twice. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:146: New []string On 2012/03/28 00:28:59, niemeyer wrote: > s/Del/Removed/ > s/New/Added/ > > And drop comments please. ChildrenChange.Added/Removed is clear. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:150: // children and delivers those via the Change() method. On 2012/03/28 00:28:59, niemeyer wrote: > // ChildrenWatcher observes a ZooKeeper node and delivers a > // notification when child nodes are added or removed. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:159: // NewWatcher creates a new watcher. On 2012/03/28 00:28:59, niemeyer wrote: > Comment is wrong, but may actually be dropped. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:172: return w, nil On 2012/03/28 00:28:59, niemeyer wrote: > If there are no errors, no need for an error result. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:176: // the node on the returned channel each time they are changing. On 2012/03/28 00:28:59, niemeyer wrote: > // Changes returns a channel that will receive the changes > // performed to the set of children of the watched node. > // Note that multiple changes may be observed as a single > // event in the channel. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:183: // are added or deleted and emits these changes via the changeChan. On 2012/03/28 00:28:59, niemeyer wrote: > Needs a new comment, and the comment should not mention the code flow. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:184: func (w *ChildrenWatcher) update(event zookeeper.Event) (nextWatch <-chan zookeeper.Event, cont bool) { On 2012/03/28 00:28:59, niemeyer wrote: > Please return error rather than bool, and fix appropriately. Done. https://codereview.appspot.com/5905064/diff/13001/state/watcher/watcher.go#ne... state/watcher/watcher.go:224: // ChildrenWatcher it just closes the changeChan. On 2012/03/28 00:28:59, niemeyer wrote: > Comment needs love. Done.
Sign in to reply to this message.
looking good. mostly minors. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:20: func NewContentWatcher(zk *zookeeper.Conn, path string) *ContentWatcher { needs a comment. // NewContentWatcher creates a new ContentWatcher // observing zookeeper node at the given path. ? https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:37: // Stop ends the watching. i think this needs a bit more because it's an important method. something like this perhaps? // Stop stops the watch and returns any error encountered // while watching. This method should always be called before // discarding the watcher. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:44: func (w *ContentWatcher) loop(eventType int) { there's no need for the eventType argument here - we know that we always want to update with an initial CHANGED event. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:48: watch, err := w.update(eventType) s/eventType/zookeeper.EVENT_CHANGED/ https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:49: if err != nil { i think you should check watch == nil here. it happens that it will actually work when watch is nil (the tomb is already dying, and we can read from Dying twice, and the nil watch channel is ok to use in the select), but better to be more direct, i think. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:73: // channel. It returns the next watch. s/\./ if it has changed./ https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:111: func NewChildrenWatcher(zk *zookeeper.Conn, path string) *ChildrenWatcher { needs a doc comment. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:130: // Stop ends the watching. expanded docs as per Stop method above. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:137: func (w *ChildrenWatcher) loop(eventType int) { no eventType necessary, as above. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:141: watch, err := w.update(eventType) s/eventType/zookeeper.EVENT_CHILD/ https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:142: if err != nil { if watch == nil { as above. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:196: sort.Strings(change.Deleted) i don't think it's worth sorting these. the client can sort them if it needs to.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:20: func NewContentWatcher(zk *zookeeper.Conn, path string) *ContentWatcher { On 2012/03/28 12:10:59, rog wrote: > needs a comment. > // NewContentWatcher creates a new ContentWatcher > // observing zookeeper node at the given path. > ? Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:37: // Stop ends the watching. On 2012/03/28 12:10:59, rog wrote: > i think this needs a bit more because it's an important method. something like > this perhaps? > > // Stop stops the watch and returns any error encountered > // while watching. This method should always be called before > // discarding the watcher. Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:44: func (w *ContentWatcher) loop(eventType int) { On 2012/03/28 12:10:59, rog wrote: > there's no need for the eventType argument here - we know that we always want to > update with an initial CHANGED event. Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:48: watch, err := w.update(eventType) On 2012/03/28 12:10:59, rog wrote: > s/eventType/zookeeper.EVENT_CHANGED/ Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:49: if err != nil { On 2012/03/28 12:10:59, rog wrote: > i think you should check watch == nil here. > > it happens that it will actually work when watch is nil (the tomb is already > dying, and we can read from Dying twice, and the nil watch channel is ok to use > in the select), but better to be more direct, i think. Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:73: // channel. It returns the next watch. On 2012/03/28 12:10:59, rog wrote: > s/\./ if it has changed./ Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:111: func NewChildrenWatcher(zk *zookeeper.Conn, path string) *ChildrenWatcher { On 2012/03/28 12:10:59, rog wrote: > needs a doc comment. Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:130: // Stop ends the watching. On 2012/03/28 12:10:59, rog wrote: > expanded docs as per Stop method above. Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:137: func (w *ChildrenWatcher) loop(eventType int) { On 2012/03/28 12:10:59, rog wrote: > no eventType necessary, as above. Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:141: watch, err := w.update(eventType) On 2012/03/28 12:10:59, rog wrote: > s/eventType/zookeeper.EVENT_CHILD/ Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:142: if err != nil { On 2012/03/28 12:10:59, rog wrote: > if watch == nil { > > as above. Done. https://codereview.appspot.com/5905064/diff/13005/state/watcher/watcher.go#ne... state/watcher/watcher.go:196: sort.Strings(change.Deleted) On 2012/03/28 12:10:59, rog wrote: > i don't think it's worth sorting these. > the client can sort them if it needs to. The costs are very low, the behavior is reproducable and predictable.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/5905064/diff/12007/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/12007/state/watcher/watcher.go#ne... state/watcher/watcher.go:76: // channel. It returns the next watch if it has changed. Not quite. // update retrieves the node content and emits it to the change // channel if it has changed. It returns the next watch. https://codereview.appspot.com/5905064/diff/12007/state/watcher/watcher.go#ne... state/watcher/watcher.go:116: func NewChildrenWatcher(zk *zookeeper.Conn, watchedPath string) *ChildrenWatcher { path would still be a fine name here BTW.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/5905064/diff/12007/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/12007/state/watcher/watcher.go#ne... state/watcher/watcher.go:76: // channel. It returns the next watch if it has changed. On 2012/03/28 13:36:08, rog wrote: > Not quite. > // update retrieves the node content and emits it to the change > // channel if it has changed. It returns the next watch. Done. https://codereview.appspot.com/5905064/diff/12007/state/watcher/watcher.go#ne... state/watcher/watcher.go:116: func NewChildrenWatcher(zk *zookeeper.Conn, watchedPath string) *ChildrenWatcher { On 2012/03/28 13:36:08, rog wrote: > path would still be a fine name here BTW. Would be fine, yes, but so it's even more expressive.
Sign in to reply to this message.
LGTM, with the following trivial. Thanks for all the changes Frank. This is nice. https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:52: if watch == nil { Why isn't that "err != nil", as usual? https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:67: if watch == nil { Same. https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:149: if watch == nil { Same. https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:164: if watch == nil { Same.
Sign in to reply to this message.
https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:52: if watch == nil { On 2012/03/28 20:08:42, niemeyer wrote: > Why isn't that "err != nil", as usual? i'm responsible for that, but i think it's correct. if someone calls Stop on the watcher, we want it to finish without an error. update will detect Dying and return (nil, nil), then we'll do w.tomb.Kill(nil) which will have no effect and all finishes normally. one alternative might be to return a special error from update (errDying) and then add; if err != errDying { w.tomb.Kill(err) } doesn't really seem worth it though to me but YMMV. another possibility would be to have the update do the Kill itself, but that's getting back towards returning (cont bool)...
Sign in to reply to this message.
https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:52: if watch == nil { On 2012/03/28 21:27:56, rog wrote: > On 2012/03/28 20:08:42, niemeyer wrote: > > Why isn't that "err != nil", as usual? > > i'm responsible for that, but i think it's correct. > > if someone calls Stop on the watcher, we want it to finish without an error. > update will detect Dying and return (nil, nil), then we'll do w.tomb.Kill(nil) > which will have no effect and all finishes normally. Returning no error with no result either is error prone as we're used to see "if err != nil { bad }" all they long. We need to find a way to avoid it. In fact, such a way was already in the tomb package before we refactored it. When I was coding with the tomb package for a toy project, I faced exactly the same problem, and that's where the tomb.Stop convention came from. What we did to the tomb package with your suggestions was a definite improvement, but since I forgot why Stop existed we ignored the use case too. > one alternative might be to return a special error from update (errDying) and > then add; This is a problem we'll face whenever we use the tomb package, so I suggest we introduce a simple mechanism within the tomb package itself that helps using the conventions we're used to in Go: let's add tomb.Dying, created as errors.New("tomb: dying"), and let's make sure that whenever Kill() is called with Dying, it checks to see if the tomb is dying, and does not change the error. If it wasn't dying, it should panic, since it's a logic mistake. > if err != errDying { > w.tomb.Kill(err) > } With the suggested approach, this would be: if err != nil { w.tomb.Kill(err) return } Which is closer to the usual convention, and doesn't have to be special cased.
Sign in to reply to this message.
Btw, Frank, this is still good to go right now. Any changes may be done later.
Sign in to reply to this message.
https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:52: if watch == nil { On 2012/03/29 02:45:38, niemeyer wrote: > On 2012/03/28 21:27:56, rog wrote: > > On 2012/03/28 20:08:42, niemeyer wrote: > > > Why isn't that "err != nil", as usual? > > > > i'm responsible for that, but i think it's correct. > > > > if someone calls Stop on the watcher, we want it to finish without an error. > > update will detect Dying and return (nil, nil), then we'll do w.tomb.Kill(nil) > > which will have no effect and all finishes normally. > > Returning no error with no result either is error prone as we're used to see "if > err != nil { bad }" all they long. We need to find a way to avoid it. In fact, > such a way was already in the tomb package before we refactored it. i'm not sure. returning a nil watcher to signify "there's nothing more to watch" seems fine to me. there is precedent for using the return value before looking at the error (io.Reader). if there's anything wrong with this code, i think it's that it does Kill(err) even when the tomb is already dying. i still think that we've got a very good way to say "no error" - it's nil. making special values that aren't "really" errors seems odd to me, although, as with recover, they can be used usefully internally. making tomb.Dying a special case means only that update will return tomb.Dying rather than errDying and it will be transparently dropped by tomb.Kill. better to keep that logic up front, i think.
Sign in to reply to this message.
*** Submitted: Watcher package as base for all state watcher. Like the presence package observing the presence of a ZooKeeper node the Watcher can be configured to observe content or children changes of nodes. Those changes are continuously delivered until the Watcher is stopped. R=niemeyer, rog CC= https://codereview.appspot.com/5905064
Sign in to reply to this message.
https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go File state/watcher/watcher.go (right): https://codereview.appspot.com/5905064/diff/12009/state/watcher/watcher.go#ne... state/watcher/watcher.go:52: if watch == nil { On 2012/03/29 10:02:44, rog wrote: > i still think that we've got a very good way to say "no error" - it's nil. That's the crux. There's an error. This is the purpose of update: // update retrieves the node content and emits it to the change // channel if it has changed. It returns the next watch. Has the node been emitted to the change channel? No. Has the next watch been returned? No. This is very clearly an exceptional situation that interrupted the normal flow of the function. The way to say that in Go is to return an error. What's the error? The tomb is in a dying state. tomb.Dying. I've experienced the use of a nil err in exactly that situation in larger scale, with the pattern you're suggesting, and it goes bad. It breaks completely the convention people are used to, and the code becomes brittle and hard to follow.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
