Adding of ServiceConfigWatcher.

Adding of ServiceConfigWatcher.

The ServiceConfigWatcher is the first one of a number of
watchers and a proof of concept. There will be similarities
between all watchers, so maybe one util type taking an
interface based behavior that's internally used is a
better approach.

https://code.launchpad.net/~themue/juju/go-state-service-config-watcher/+merge/99027

(do not edit description out of merge proposal)

[TheMue, 2012-03-23 14:07:20]

Please take a look.

[TheMue, 2012-03-30 09:12:09]

Please take a look.

[rog, 2012-03-30 10:04:22]

looks great. mostly minor comments.

https://codereview.appspot.com/5885059/diff/3001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode18
state/service.go:18: type ServiceConfigWatcher struct {
this is beautifully concise. yay!

unfortunately it's perhaps a little too concise - it shouldn't send to w.changes
without guarding for Stop. i guess that might mean we want to expose the tomb
within watcher.ContentWatcher, or a method that exposes the Dying channel at any
rate.

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode24
state/service.go:24: // Changes returns a channel delivering the changed service
configurations.
s/a channel/the channel/

i think it's useful to imply that Changes always returns the same channel.

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode35
state/service.go:35: func (w *ServiceConfigWatcher) loop() {
defer close(w.changes)

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode42
state/service.go:42: w.changes <- configNode
select {
case w.changes <- configNode:
case <-dying:
    return
}

https://codereview.appspot.com/5885059/diff/3001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5885059/diff/3001/state/util.go#newcode92
state/util.go:92: if err := goyaml.Unmarshal([]byte(content), c.cache); err !=
nil {
you could save a line of code (the make above) by using &c.cache here.

https://codereview.appspot.com/5885059/diff/3001/state/util.go#newcode95
state/util.go:95: c.pristineCache = copyCache(c.cache)
i know this isn't directly related to this CL, but it seems a little excessive
to create two copies of the map on every event. maybe the modification methods
could do a copy-on-write, or something?

https://codereview.appspot.com/5885059/diff/3001/state/util.go#newcode309
state/util.go:309: // singleEventChan creates a ZooKeeper event channel with an
i don't think this is used

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode7
state/watch_test.go:7: 
it would be nice to test the error path too.

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode37
state/watch_test.go:37: c.Assert(changedConfig.Keys(), HasLen, 2)
c.Assert(changedConfig.Map(), DeepEquals, map[string]interface{}{"foo": "bar",
"baz": "yadda"})
?

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode43
state/watch_test.go:43: c.Assert(changedConfig.Keys(), HasLen, 1)
c.Assert(changedConfig.Map(), DeepEquals, map[string]interface{}{"baz":
"yadda"})
?

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode54
state/watch_test.go:54: case <-time.After(2 * time.Second):
2 seconds seems like a very long time. 200ms would probably be enough.

[TheMue, 2012-03-30 14:48:58]

Please take a look.

https://codereview.appspot.com/5885059/diff/3001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode18
state/service.go:18: type ServiceConfigWatcher struct {
On 2012/03/30 10:04:22, rog wrote:
> this is beautifully concise. yay!
> 
> unfortunately it's perhaps a little too concise - it shouldn't send to
w.changes
> without guarding for Stop. i guess that might mean we want to expose the tomb
> within watcher.ContentWatcher, or a method that exposes the Dying channel at
any
> rate.

Done. Chosen to expose the Dying() channel.

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode24
state/service.go:24: // Changes returns a channel delivering the changed service
configurations.
On 2012/03/30 10:04:22, rog wrote:
> s/a channel/the channel/
> 
> i think it's useful to imply that Changes always returns the same channel.

Done.

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode35
state/service.go:35: func (w *ServiceConfigWatcher) loop() {
On 2012/03/30 10:04:22, rog wrote:
> defer close(w.changes)

Done.

https://codereview.appspot.com/5885059/diff/3001/state/service.go#newcode42
state/service.go:42: w.changes <- configNode
On 2012/03/30 10:04:22, rog wrote:
> select {
> case w.changes <- configNode:
> case <-dying:
>     return
> }

Done.

https://codereview.appspot.com/5885059/diff/3001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5885059/diff/3001/state/util.go#newcode92
state/util.go:92: if err := goyaml.Unmarshal([]byte(content), c.cache); err !=
nil {
On 2012/03/30 10:04:22, rog wrote:
> you could save a line of code (the make above) by using &c.cache here.

Done.

https://codereview.appspot.com/5885059/diff/3001/state/util.go#newcode95
state/util.go:95: c.pristineCache = copyCache(c.cache)
On 2012/03/30 10:04:22, rog wrote:
> i know this isn't directly related to this CL, but it seems a little excessive
> to create two copies of the map on every event. maybe the modification methods
> could do a copy-on-write, or something?

Will keep it in mind for a small extra branch.

https://codereview.appspot.com/5885059/diff/3001/state/util.go#newcode309
state/util.go:309: // singleEventChan creates a ZooKeeper event channel with an
On 2012/03/30 10:04:22, rog wrote:
> i don't think this is used

Done.

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode7
state/watch_test.go:7: 
On 2012/03/30 10:04:22, rog wrote:
> it would be nice to test the error path too.

Yip, already planned to do so.

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode37
state/watch_test.go:37: c.Assert(changedConfig.Keys(), HasLen, 2)
On 2012/03/30 10:04:22, rog wrote:
> c.Assert(changedConfig.Map(), DeepEquals, map[string]interface{}{"foo": "bar",
> "baz": "yadda"})
> ?

Done.

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode43
state/watch_test.go:43: c.Assert(changedConfig.Keys(), HasLen, 1)
On 2012/03/30 10:04:22, rog wrote:
> c.Assert(changedConfig.Map(), DeepEquals, map[string]interface{}{"baz":
> "yadda"})
> ?

Done.

https://codereview.appspot.com/5885059/diff/3001/state/watch_test.go#newcode54
state/watch_test.go:54: case <-time.After(2 * time.Second):
On 2012/03/30 10:04:22, rog wrote:
> 2 seconds seems like a very long time. 200ms would probably be enough.

Done.

[rog, 2012-03-30 15:03:40]

LGTM

https://codereview.appspot.com/5885059/diff/7001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5885059/diff/7001/state/service.go#newcode17
state/service.go:17: // ServiceConfigWatcher observices configuration changes of
a service.
observices? :-)
reads a bit awkwardly. perhaps this?

// ServiceConfigWatcher observes changes to the configuration
// of a service.

https://codereview.appspot.com/5885059/diff/7001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/5885059/diff/7001/state/watcher/watcher.go#new...
state/watcher/watcher.go:46: // that the watcher stops working.
// Dying returns a channel that indicates
// that the watcher is about to finish.
?

https://codereview.appspot.com/5885059/diff/7001/state/watcher/watcher.go#new...
state/watcher/watcher.go:156: // that the watcher stops working.
as above.

[TheMue, 2012-03-30 15:11:31]

Please take a look.

https://codereview.appspot.com/5885059/diff/7001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5885059/diff/7001/state/service.go#newcode17
state/service.go:17: // ServiceConfigWatcher observices configuration changes of
a service.
On 2012/03/30 15:03:40, rog wrote:
> observices? :-)
> reads a bit awkwardly. perhaps this?
> 
> // ServiceConfigWatcher observes changes to the configuration
> // of a service.

Done. What a crazy typo! ;)

https://codereview.appspot.com/5885059/diff/7001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/5885059/diff/7001/state/watcher/watcher.go#new...
state/watcher/watcher.go:46: // that the watcher stops working.
On 2012/03/30 15:03:40, rog wrote:
> // Dying returns a channel that indicates
> // that the watcher is about to finish.
> ?

Done.

https://codereview.appspot.com/5885059/diff/7001/state/watcher/watcher.go#new...
state/watcher/watcher.go:156: // that the watcher stops working.
On 2012/03/30 15:03:40, rog wrote:
> as above.

Done.

[niemeyer, 2012-04-03 17:10:52]

Looks very nice overall.

https://codereview.appspot.com/5885059/diff/9001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5885059/diff/9001/state/service.go#newcode19
state/service.go:19: type ServiceConfigWatcher struct {
This isn't this a *ConfigWatcher*?  Is there anything that makes it specially
tailored to services? It looks like it just needs a path?

https://codereview.appspot.com/5885059/diff/9001/state/service.go#newcode42
state/service.go:42: w.watcher.Kill(err)
This isn't good, as we discussed. It's using the watcher as a bin for errors
that are not coming from its logic. Just use a local tomb for that. It's fine to
have Dying() in the watcher, though, so we can unblock on *real* underlying
errors originated there.

https://codereview.appspot.com/5885059/diff/9001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5885059/diff/9001/state/util.go#newcode87
state/util.go:87: // not implicit written to ZooKeeper.
s/implicit/implicitly/, although I'm still not sure of what it means. Can you
please expand the command a bit?

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:38: // Kill allows a pipelined goroutine to stop the
watcher
As we discussed online, this method does not look good. This is a
ContentWatcher, not a Tomb. It's purpose is to watch content, not to be a
conduit for errors that it has absolutely no clue about.

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:46: // that the watcher is about to finish.
This one is fine, though, as we can legitimately observe errors that happened
within the watcher itself with it. Please just tweak the comment a bit to make
it more precise:

// Dying returns a channel that is closed when the
// watcher has stopped or is about to stop.

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:150: // an illegal format or state.
Same.

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:156: // that the watcher is about to finish.
Same.

[TheMue, 2012-04-04 13:53:19]

Please take a look.

https://codereview.appspot.com/5885059/diff/9001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5885059/diff/9001/state/service.go#newcode19
state/service.go:19: type ServiceConfigWatcher struct {
On 2012/04/03 17:10:52, niemeyer wrote:
> This isn't this a *ConfigWatcher*?  Is there anything that makes it specially
> tailored to services? It looks like it just needs a path?

Done.

https://codereview.appspot.com/5885059/diff/9001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5885059/diff/9001/state/util.go#newcode87
state/util.go:87: // not implicit written to ZooKeeper.
On 2012/04/03 17:10:52, niemeyer wrote:
> s/implicit/implicitly/, although I'm still not sure of what it means. Can you
> please expand the command a bit?

Done. Just removed the second sentence, it is indeed misleading.

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:38: // Kill allows a pipelined goroutine to stop the
watcher
On 2012/04/03 17:10:52, niemeyer wrote:
> As we discussed online, this method does not look good. This is a
> ContentWatcher, not a Tomb. It's purpose is to watch content, not to be a
> conduit for errors that it has absolutely no clue about.

Done.

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:46: // that the watcher is about to finish.
On 2012/04/03 17:10:52, niemeyer wrote:
> This one is fine, though, as we can legitimately observe errors that happened
> within the watcher itself with it. Please just tweak the comment a bit to make
> it more precise:
> 
> // Dying returns a channel that is closed when the
> // watcher has stopped or is about to stop.

Done.

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:150: // an illegal format or state.
On 2012/04/03 17:10:52, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5885059/diff/9001/state/watcher/watcher.go#new...
state/watcher/watcher.go:156: // that the watcher is about to finish.
On 2012/04/03 17:10:52, niemeyer wrote:
> Same.

Done.

[niemeyer, 2012-04-05 00:15:35]

Thanks for the changes. Another round, and probably the last.

https://codereview.appspot.com/5885059/diff/4016/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5885059/diff/4016/state/state.go#newcode264
state/state.go:264: // Changes returns the channel delivering the changed
configurations.
// Changes returns a channel that will receive the new
// *ConfigNode when a change is detected. Note that multiple
// changes may be observed as a single event in the channel.

https://codereview.appspot.com/5885059/diff/4016/state/state.go#newcode269
state/state.go:269: // Stop ends the configuration watching.
// Stop stops the watch and returns any error encountered
// while watching. This method should always be called
// before discarding the watcher.

If these comments do not make sense, or should be improved,
please do so in all the watchers at the same time.

https://codereview.appspot.com/5885059/diff/4016/state/state.go#newcode273
state/state.go:273: return err
It's cool to give precedence to the watcher error, but it must still wait before
its own loop terminates before returning.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode17
state/watch_test.go:17: watcher := wordpress.WatchConfig()
Nice, this is getting quite readable.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode24
state/watch_test.go:24: time.Sleep(100 * time.Millisecond)
I believe the goroutine is not necessary here, nor the sleep above. The watch
should be receiving the changes in background and acting on them.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode27
state/watch_test.go:27: config.Write()
err := ...
c.Assert(err, IsNil)

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode29
state/watch_test.go:29: time.Sleep(100 * time.Millisecond)
This sleep should stay, though, to ensure you get the two changes independently.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode31
state/watch_test.go:31: config.Write()
err := ...
c.Assert(err, IsNil)

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode52
state/watch_test.go:52: c.Fail()
c.Fatalf with a proper message, please.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode78
state/watch_test.go:78: c.Fail()
c.Fatalf with a proper message, please.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode82
state/watch_test.go:82: c.Assert(err, ErrorMatches, "YAML error: .*")
Nice behavior.

https://codereview.appspot.com/5885059/diff/4016/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/5885059/diff/4016/state/watcher/watcher.go#new...
state/watcher/watcher.go:97: return nil, tomb.ErrDying
Thanks, but if you're going there please replace the watch == nil as well.

[TheMue, 2012-04-05 13:02:42]

Please take a look.

https://codereview.appspot.com/5885059/diff/4016/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5885059/diff/4016/state/state.go#newcode264
state/state.go:264: // Changes returns the channel delivering the changed
configurations.
On 2012/04/05 00:15:35, niemeyer wrote:
> // Changes returns a channel that will receive the new
> // *ConfigNode when a change is detected. Note that multiple
> // changes may be observed as a single event in the channel.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/state.go#newcode269
state/state.go:269: // Stop ends the configuration watching.
On 2012/04/05 00:15:35, niemeyer wrote:
> // Stop stops the watch and returns any error encountered
> // while watching. This method should always be called
> // before discarding the watcher.
> 
> If these comments do not make sense, or should be improved,
> please do so in all the watchers at the same time.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/state.go#newcode273
state/state.go:273: return err
On 2012/04/05 00:15:35, niemeyer wrote:
> It's cool to give precedence to the watcher error, but it must still wait
before
> its own loop terminates before returning.

Done. Difficult situation would be if in case of an internal watcher error
Dying() returns an error too. Haven't been able to discaver that in the control
flow.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode17
state/watch_test.go:17: watcher := wordpress.WatchConfig()
On 2012/04/05 00:15:35, niemeyer wrote:
> Nice, this is getting quite readable.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode24
state/watch_test.go:24: time.Sleep(100 * time.Millisecond)
On 2012/04/05 00:15:35, niemeyer wrote:
> I believe the goroutine is not necessary here, nor the sleep above. The watch
> should be receiving the changes in background and acting on them.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode27
state/watch_test.go:27: config.Write()
On 2012/04/05 00:15:35, niemeyer wrote:
> err := ...
> c.Assert(err, IsNil)

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode29
state/watch_test.go:29: time.Sleep(100 * time.Millisecond)
On 2012/04/05 00:15:35, niemeyer wrote:
> This sleep should stay, though, to ensure you get the two changes
independently.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode31
state/watch_test.go:31: config.Write()
On 2012/04/05 00:15:35, niemeyer wrote:
> err := ...
> c.Assert(err, IsNil)

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode52
state/watch_test.go:52: c.Fail()
On 2012/04/05 00:15:35, niemeyer wrote:
> c.Fatalf with a proper message, please.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode78
state/watch_test.go:78: c.Fail()
On 2012/04/05 00:15:35, niemeyer wrote:
> c.Fatalf with a proper message, please.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watch_test.go#newcode82
state/watch_test.go:82: c.Assert(err, ErrorMatches, "YAML error: .*")
On 2012/04/05 00:15:35, niemeyer wrote:
> Nice behavior.

Done.

https://codereview.appspot.com/5885059/diff/4016/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/5885059/diff/4016/state/watcher/watcher.go#new...
state/watcher/watcher.go:97: return nil, tomb.ErrDying
On 2012/04/05 00:15:35, niemeyer wrote:
> Thanks, but if you're going there please replace the watch == nil as well.

Done.

[niemeyer, 2012-04-05 14:28:07]

LGTM, thank you.

[TheMue, 2012-04-05 14:43:16]

*** Submitted:

Adding of ServiceConfigWatcher.

The ServiceConfigWatcher is the first one of a number of
watchers and a proof of concept. There will be similarities
between all watchers, so maybe one util type taking an
interface based behavior that's internally used is a
better approach.

R=rog, niemeyer
CC=
https://codereview.appspot.com/5885059