state: MachineUnitsWatcher unwatches correct units

state: MachineUnitsWatcher unwatches correct units

This means that stopped MUWs no longer end up blocking the watcher. To help
detect problems like this more easily in future, the watcher package now
panics when unwatching an unknown key.

And I fixed RelationScopeWatcher, which was using the old'n'busted double-
loop style, and replaced "changeChan"s with "out"s for those watchers that
I expect to continue to exist (ie not MachinePrincipalUnitsWatcher). Sorry,
I couldn't resist.

https://code.launchpad.net/~fwereade/juju-core/state-muw-unwatch-fix/+merge/137133

(do not edit description out of merge proposal)

[fwereade, 2012-11-30 09:38:27]

Please take a look.

[rog, 2012-11-30 09:47:17]

On 2012/11/30 09:38:27, fwereade wrote:
> Please take a look.

LGTM

is it possible to make a test for this?

now that machine id is a string, could we just change the id in state/watcher to
string rather than interface{}?

that would help catch problems like this.

[TheMue, 2012-11-30 09:47:58]

LGTM

[fwereade, 2012-11-30 11:15:42]

Please take a look.

[rog, 2012-11-30 11:21:47]

LGTM

nice to see something that would have made the tests fail.

https://codereview.appspot.com/6856122/diff/4001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6856122/diff/4001/state/watcher/watcher.go#new...
state/watcher/watcher.go:272: panic(fmt.Errorf("tried to re-add channel %v for
%s", info.ch, r.key))
"cannot watch %v twice", r.key

?

https://codereview.appspot.com/6856122/diff/4001/state/watcher/watcher.go#new...
state/watcher/watcher.go:292: panic(fmt.Errorf("tried to remove missing channel
%v for %s", r.ch, r.key))
i'm not sure that printing the channel pointer value helps here. how about
("cannot find %v to unwatch it", r.key) ?

[TheMue, 2012-11-30 11:30:50]

LGTM, and supporting Rogers comments.

https://codereview.appspot.com/6856122/diff/4001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6856122/diff/4001/state/watcher/watcher.go#new...
state/watcher/watcher.go:67: func (k watchKey) String() string {
Nice way for readable error messages.

https://codereview.appspot.com/6856122/diff/4001/state/watcher/watcher.go#new...
state/watcher/watcher.go:272: panic(fmt.Errorf("tried to re-add channel %v for
%s", info.ch, r.key))
On 2012/11/30 11:21:47, rog wrote:
> "cannot watch %v twice", r.key
> 
> ?

+1

https://codereview.appspot.com/6856122/diff/4001/state/watcher/watcher.go#new...
state/watcher/watcher.go:292: panic(fmt.Errorf("tried to remove missing channel
%v for %s", r.ch, r.key))
On 2012/11/30 11:21:47, rog wrote:
> i'm not sure that printing the channel pointer value helps here. how about
> ("cannot find %v to unwatch it", r.key) ?

And also here +1.

[ricky.kernberger, 2012-11-30 11:34:11]

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

https://codereview.appspot.com/6856122/diff/1/state/watcher.go#oldcode1311
state/watcher.go:1311: for _, unit := range w.known {
murks

[fwereade, 2012-11-30 11:40:42]

*** Submitted:

state: MachineUnitsWatcher unwatches correct units

This means that stopped MUWs no longer end up blocking the watcher. To help
detect problems like this more easily in future, the watcher package now
panics when unwatching an unknown key.

And I fixed RelationScopeWatcher, which was using the old'n'busted double-
loop style, and replaced "changeChan"s with "out"s for those watchers that
I expect to continue to exist (ie not MachinePrincipalUnitsWatcher). Sorry,
I couldn't resist.

R=ricky.kernberger
CC=
https://codereview.appspot.com/6856122