mstate: add machines watcher

mstate: add machines watcher



https://code.launchpad.net/~aramh/juju-core/71-mstate-watchers-machines-nwo/+merge/124711

(do not edit description out of merge proposal)

[aram, 2012-09-17 15:15:26]

Please take a look.

[aram, 2012-09-17 15:29:41]

Please take a look.

[niemeyer, 2012-09-17 15:39:49]

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode136
mstate/watcher.go:136: func (w *MachinesWatcher) appendChange(changes
*MachinesChange, ch watcher.Change) (err error) {
s/append/merge/

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode138
mstate/watcher.go:138: if m, ok := w.knownMachines[id]; ch.Revno == -1 && ok {
We actually don't have to report when we get -1. One of the critical points of
Dead is precisely that it enables us to tell interested parties that the machine
is going away *before* it actually goes away. When it does go away, we don't
need to report it anymore, because parties already got Dying and Dead before
that.

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode145
mstate/watcher.go:145: err = w.st.machines.FindId(id).One(doc)
The semantic above also means that we don't need to load the document every
single time we get a report, which would be a poor approach. It'd mean anyone
monitoring machines added or removed load every single machine every single time
each of them change in any way. We just need to track id => bool, and load when
we want to report.

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode167
mstate/watcher.go:167: err = w.st.machines.Find(nil).All(&docs)
This will do for now, but some day we should really have an iterator coming out
of the change report instead.

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode188
mstate/watcher.go:188: if changes == nil {
The logic may be simplified a bit by organizing the selects as:

for !change.isEmpty() {
     (... select that sends ...)
}
(... select that doesn't send ...)

This drops the nil test, the niling out of changes, and the test for isEmpty
within the select.

[aram, 2012-09-17 16:02:54]

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode136
mstate/watcher.go:136: func (w *MachinesWatcher) appendChange(changes
*MachinesChange, ch watcher.Change) (err error) {
On 2012/09/17 15:39:49, niemeyer wrote:
> s/append/merge/

Done.

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode138
mstate/watcher.go:138: if m, ok := w.knownMachines[id]; ch.Revno == -1 && ok {
On 2012/09/17 15:39:49, niemeyer wrote:
> We actually don't have to report when we get -1. One of the critical points of
> Dead is precisely that it enables us to tell interested parties that the
machine
> is going away *before* it actually goes away. When it does go away, we don't
> need to report it anymore, because parties already got Dying and Dead before
> that.

I know, but I did it this way to be compatible with state. In the future we will
not need this.

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode145
mstate/watcher.go:145: err = w.st.machines.FindId(id).One(doc)
On 2012/09/17 15:39:49, niemeyer wrote:
> The semantic above also means that we don't need to load the document every
> single time we get a report, which would be a poor approach. It'd mean anyone
> monitoring machines added or removed load every single machine every single
time
> each of them change in any way. We just need to track id => bool, and load
when
> we want to report.

Discussed at Lisbon.

https://codereview.appspot.com/6496131/diff/1/mstate/watcher.go#newcode188
mstate/watcher.go:188: if changes == nil {
On 2012/09/17 15:39:49, niemeyer wrote:
> The logic may be simplified a bit by organizing the selects as:
> 
> for !change.isEmpty() {
>      (... select that sends ...)
> }
> (... select that doesn't send ...)
> 
> This drops the nil test, the niling out of changes, and the test for isEmpty
> within the select.

Done.

[aram, 2012-09-17 16:03:41]

*** Submitted:

mstate: add machines watcher

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

[niemeyer, 2012-09-17 16:16:56]

Pretty much ready. Just a trivial on the test and a question.

https://codereview.appspot.com/6496131/diff/5002/mstate/state_test.go
File mstate/state_test.go (right):

https://codereview.appspot.com/6496131/diff/5002/mstate/state_test.go#newcode217
mstate/state_test.go:217: // 0
Such indexes make it inconvenient to organize and add tests, for little gain. If
you want to make it more debuggable, the best way is having a "summary string"
field at the top of the struct, and adding inline comments describing what each
test is doing.

https://codereview.appspot.com/6496131/diff/5002/mstate/state_test.go#newcode347
mstate/state_test.go:347: if moreMachinesRequired(got, test.added, test.removed)
{
Given the use of StartSync above, I believe we can guarantee the semantics
without this. Does it break if you just assert below?

https://codereview.appspot.com/6496131/diff/5002/mstate/state_test.go#newcode382
mstate/state_test.go:382: c.Assert(len(got.Removed), Equals, len(removed))
Instead of doing this, I suggest building two sorted lists of []int, and
asserting one against the other. This gives us good debugging info when it
fails.