state: simplify entity watchers

state: simplify entity watchers

As long agreed but never implemented, service and unit watchers now just
report the fact of a change but elide the details.

Settings changes are not fixed in this fashion: that'll be a bit more
complicated and there's no pressing need.

https://code.launchpad.net/~fwereade/juju-core/drop-unwanted-watcher-entities/+merge/133280

(do not edit description out of merge proposal)

[fwereade, 2012-11-07 15:36:51]

Please take a look.

[dave_cheney.net, 2012-11-07 23:44:40]

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

https://codereview.appspot.com/6811091/diff/1/state/service_test.go#newcode978
state/service_test.go:978: err = s.service.Refresh()
err :=

this is a new block

https://codereview.appspot.com/6811091/diff/1/state/service_test.go#newcode996
state/service_test.go:996: err = s.service.Refresh()
ditto

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

https://codereview.appspot.com/6811091/diff/1/state/unit_test.go#newcode481
state/unit_test.go:481: err = s.unit.Refresh()
err :=

https://codereview.appspot.com/6811091/diff/1/state/unit_test.go#newcode499
state/unit_test.go:499: err = s.unit.Refresh()
ditto

https://codereview.appspot.com/6811091/diff/1/state/watcher.go
File state/watcher.go (left):

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#oldcode1123
state/watcher.go:1123: // Multiple changes may be observed as a single event in
the channel.
Is this comment no longer true ? I know rogpeppe was very fond of it.

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

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1092
state/watcher.go:1092: out = nil
interesting pattern.

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1151
state/watcher.go:1151: 
Moving this down here makes it harder to read the side by side diff. Has
anything changed in this block of code ?

[rog, 2012-11-09 10:48:58]

LGTM with minor remarks below

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

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1070
state/watcher.go:1070: func (w *UnitWatcher) Changes() <-chan string {
comment?

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1151
state/watcher.go:1151: 
On 2012/11/07 23:44:41, dfc wrote:
> Moving this down here makes it harder to read the side by side diff. Has
> anything changed in this block of code ?

+1

https://codereview.appspot.com/6811091/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6811091/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:546: ports = append([]state.Port(nil),
change...)
s/[]state.Port(nil)/ports[:0]/

no need for unnecessary allocation AFAICS.

https://codereview.appspot.com/6811091/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:600: go sd.watchLoop(sd.exposed)
perhaps we could consider using the pattern that's becoming prevalent elsewhere
- make watchLoop return an error, and kill the tomb with that error. this means
the tomb.Kill lines below can just return the error.

[niemeyer, 2012-11-14 16:56:14]

LGTM

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

https://codereview.appspot.com/6811091/diff/1/state/service_test.go#newcode978
state/service_test.go:978: err = s.service.Refresh()
On 2012/11/07 23:44:41, dfc wrote:
> err :=
> 
> this is a new block

FWIW, I don't think we've been strict about this elsewhere, and I'm not sure if
there's any benefit in being in this case.

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

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1070
state/watcher.go:1070: func (w *UnitWatcher) Changes() <-chan string {
// Changes returns the event channel for the UnitWatcher.

That's equivalent to the other watchers. If we want to detail the behavior of
the watcher, let's please do that in the type doc.

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1151
state/watcher.go:1151: 
+1; ideally moves and changes happen in different CLs.

I'm assuming this has not changed.

[fwereade, 2012-11-19 09:07:20]

*** Submitted:

state: simplify entity watchers

As long agreed but never implemented, service and unit watchers now just
report the fact of a change but elide the details.

Settings changes are not fixed in this fashion: that'll be a bit more
complicated and there's no pressing need.

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

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

https://codereview.appspot.com/6811091/diff/1/state/service_test.go#newcode978
state/service_test.go:978: err = s.service.Refresh()
On 2012/11/14 16:56:15, niemeyer wrote:
> On 2012/11/07 23:44:41, dfc wrote:
> > err :=
> > 
> > this is a new block
> 
> FWIW, I don't think we've been strict about this elsewhere, and I'm not sure
if
> there's any benefit in being in this case.

I'm fine changing it if it improves the reading experience :).

https://codereview.appspot.com/6811091/diff/1/state/service_test.go#newcode996
state/service_test.go:996: err = s.service.Refresh()
On 2012/11/07 23:44:41, dfc wrote:
> ditto

Done.

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

https://codereview.appspot.com/6811091/diff/1/state/unit_test.go#newcode481
state/unit_test.go:481: err = s.unit.Refresh()
On 2012/11/07 23:44:41, dfc wrote:
> err :=

Done.

https://codereview.appspot.com/6811091/diff/1/state/unit_test.go#newcode499
state/unit_test.go:499: err = s.unit.Refresh()
On 2012/11/07 23:44:41, dfc wrote:
> ditto

Done.

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

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1070
state/watcher.go:1070: func (w *UnitWatcher) Changes() <-chan string {
On 2012/11/14 16:56:15, niemeyer wrote:
> // Changes returns the event channel for the UnitWatcher.
> 
> That's equivalent to the other watchers. If we want to detail the behavior of
> the watcher, let's please do that in the type doc.

Done.

https://codereview.appspot.com/6811091/diff/1/state/watcher.go#newcode1151
state/watcher.go:1151: 
On 2012/11/14 16:56:15, niemeyer wrote:
> +1; ideally moves and changes happen in different CLs.
> 
> I'm assuming this has not changed.

Nothing changed; I'll try to be religious about the separation henceforth.

https://codereview.appspot.com/6811091/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6811091/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:546: ports = append([]state.Port(nil),
change...)
On 2012/11/09 10:48:58, rog wrote:
> s/[]state.Port(nil)/ports[:0]/
> 
> no need for unnecessary allocation AFAICS.

Done.

https://codereview.appspot.com/6811091/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:600: go sd.watchLoop(sd.exposed)
On 2012/11/09 10:48:58, rog wrote:
> perhaps we could consider using the pattern that's becoming prevalent
elsewhere
> - make watchLoop return an error, and kill the tomb with that error. this
means
> the tomb.Kill lines below can just return the error.

I'm not sure the pattern quite fits here -- we don't always kill on the way out
of that method.