state: handle unassignments on MachineUnitsWatcher

state: handle unassignments on MachineUnitsWatcher



https://code.launchpad.net/~niemeyer/juju-core/machine-units-watcher-unassignments/+merge/132081

(do not edit description out of merge proposal)

[niemeyer, 2012-10-30 12:02:18]

Please take a look.

[fwereade, 2012-10-31 10:45:39]

LGTM modulo suggestions below.

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

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode966
state/machine_test.go:966: charm := s.AddTestingCharm(c, "dummy")
It would be nice to use a charm other than dummy here.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode976
state/machine_test.go:976: subordinate2, err :=
subservice.AddUnitSubordinateTo(principal)
Could we perhaps have two distinct subservices here? It feels like this *should*
be an error, even if it isn't today.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode1007
state/machine_test.go:1007: s.State.Sync()
I suspect I'm missing a subtlety here: why Sync here and only StartSync above?

[aram, 2012-11-01 11:36:04]

This change is LGTM, though I am very unhappy with this particular watcher's
code in general and I think it's worth simplifying at some point.

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

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode963
state/machine_test.go:963: c.Fatalf("wacther didn't send initial event")
watcher

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode984
state/machine_test.go:984: case change := <-unitWatcher.Changes():
This should be in a loop. It's unlikely that the watcher will send two events
instead of one, but it is possible and I've seen it happening.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode988
state/machine_test.go:988: c.Fatalf("wacther didn't send expected event")
watcher

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode1007
state/machine_test.go:1007: s.State.Sync()
On 2012/10/31 10:45:40, fwereade wrote:
> I suspect I'm missing a subtlety here: why Sync here and only StartSync above?

To ensure the watcher catches the reassignment, presumably. This needs a
comment.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode1010
state/machine_test.go:1010: case change := <-unitWatcher.Changes():
loop.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode1014
state/machine_test.go:1014: c.Fatalf("wacther didn't send expected event")
watcher

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

https://codereview.appspot.com/6775076/diff/1/state/watcher.go#newcode1313
state/watcher.go:1313: pending = append(pending, unit)
hasString?

https://codereview.appspot.com/6775076/diff/1/state/watcher.go#newcode1320
state/watcher.go:1320: pending = append(pending, subunit)
hasString?

[gustavo_niemeyer.net, 2012-11-02 08:54:01]

On Thu, Nov 1, 2012 at 12:36 PM,  <aram@mgk.ro> wrote:
> This change is LGTM, though I am very unhappy with this particular
> watcher's code in general and I think it's worth simplifying at some
> point.

I've only worked on this because you told me face-to-face that there
was no way to simplify the logic that was in place in your branch [1],
after several weeks of me asking you to please simplify it [2].
Stating you're unhappy with its complexity now is very disingenuous.

[1] https://codereview.appspot.com/6720049/
[2] https://codereview.appspot.com/6625069/


gustavo @ http://niemeyer.net

[niemeyer, 2012-11-02 09:03:42]

Please take a look.

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

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode963
state/machine_test.go:963: c.Fatalf("wacther didn't send initial event")
On 2012/11/01 11:36:05, aram wrote:
> watcher

Done.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode966
state/machine_test.go:966: charm := s.AddTestingCharm(c, "dummy")
On 2012/10/31 10:45:40, fwereade wrote:
> It would be nice to use a charm other than dummy here.

Done.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode976
state/machine_test.go:976: subordinate2, err :=
subservice.AddUnitSubordinateTo(principal)
On 2012/10/31 10:45:40, fwereade wrote:
> Could we perhaps have two distinct subservices here? It feels like this
*should*
> be an error, even if it isn't today.

We don't have a good second subordinate ATM I'm afraid. Having two logging
subordinates on different services would probably be even more curious to a
casual reader. I suggest we postpone that fix for when we forbid it (it'll tell
us about the issue then).

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode984
state/machine_test.go:984: case change := <-unitWatcher.Changes():
On 2012/11/01 11:36:05, aram wrote:
> This should be in a loop. It's unlikely that the watcher will send two events
> instead of one, but it is possible and I've seen it happening.

The loop shouldn't be necessary, but this should also be a Sync.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode988
state/machine_test.go:988: c.Fatalf("wacther didn't send expected event")
On 2012/11/01 11:36:05, aram wrote:
> watcher

Done.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode1007
state/machine_test.go:1007: s.State.Sync()
On 2012/10/31 10:45:40, fwereade wrote:
> I suspect I'm missing a subtlety here: why Sync here and only StartSync above?

It is indeed somewhat subtle. The above logic has done two changes rather than
one, so I cannot just ask for it to StartSync, otherwise I could see only the
first change. With Sync, though, it means that it will wait for all of the
changes done to be observed and dispatched to the high-level watchers, so we
have a guarantee that the watcher itself is necessarily working on all of the
changes done.

I'll use Sync consistently here, though, to avoid having to figure what needs
and what doesn't.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode1010
state/machine_test.go:1010: case change := <-unitWatcher.Changes():
On 2012/11/01 11:36:05, aram wrote:
> loop.

See above.

https://codereview.appspot.com/6775076/diff/1/state/machine_test.go#newcode1014
state/machine_test.go:1014: c.Fatalf("wacther didn't send expected event")
On 2012/11/01 11:36:05, aram wrote:
> watcher

Done.

[niemeyer, 2012-11-02 10:35:57]

*** Submitted:

state: handle unassignments on MachineUnitsWatcher

R=fwereade, aram, gustavo@niemeyer.net
CC=
https://codereview.appspot.com/6775076