Small ConfigWatcher change.

Small ConfigWatcher change.

Only applying the ContentWatcher changes to the ConfigWatcher.

https://code.launchpad.net/~themue/juju/go-state-apply-content-watcher-change/+merge/102460

(do not edit description out of merge proposal)

[TheMue, 2012-04-18 08:35:20]

Please take a look.

[niemeyer, 2012-04-18 19:12:55]

This is missing the pre-req on the other branch.

The change here LGTM, but address the other branch first.

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

https://codereview.appspot.com/6056047/diff/1/state/state.go#newcode293
state/state.go:293: // A non-existing node is treaten as an empty node.
s/treaten/treated/

[niemeyer, 2012-04-18 19:14:36]

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

https://codereview.appspot.com/6056047/diff/1/state/state.go#newcode292
state/state.go:292: case change := <-w.watcher.Changes():
Actually, I believe there's a bug here. This needs an "ok" handling as well,
doesn't it? Otherwise, it will panic if the channel is closed and you try to
touch change.Content below (which will be nil).

[TheMue, 2012-04-19 11:47:29]

Please take a look.

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

https://codereview.appspot.com/6056047/diff/1/state/state.go#newcode292
state/state.go:292: case change := <-w.watcher.Changes():
On 2012/04/18 19:14:36, niemeyer wrote:
> Actually, I believe there's a bug here. This needs an "ok" handling as well,
> doesn't it? Otherwise, it will panic if the channel is closed and you try to
> touch change.Content below (which will be nil).

Done.

https://codereview.appspot.com/6056047/diff/1/state/state.go#newcode293
state/state.go:293: // A non-existing node is treaten as an empty node.
On 2012/04/18 19:12:55, niemeyer wrote:
> s/treaten/treated/

Done.

[niemeyer, 2012-04-19 11:55:31]

LGTM, assuming the following change.

https://codereview.appspot.com/6056047/diff/5001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6056047/diff/5001/state/state.go#newcode294
state/state.go:294: w.tomb.Killf("config watcher: unexpected close of changes
channel")
I believe this should be w.tomb.Kill(nil). It's fine for that channel to be
closed. How it was closed is the question, and this question will be answered
with w.watcher.Stop(), which is done on the Stop method.

[TheMue, 2012-04-19 12:09:09]

*** Submitted:

Small ConfigWatcher change.

Only applying the ContentWatcher changes to the ConfigWatcher.

R=niemeyer
CC=
https://codereview.appspot.com/6056047