state: update ServiceUnitsWatcher

state: update ServiceUnitsWatcher

Add lifecycle support to the ServiceUnitsWatcher and make it a new style
watcher that only returns ids.

https://code.launchpad.net/~aramh/juju-core/101-state-watchers-service-units5/+merge/127837

(do not edit description out of merge proposal)

[aram, 2012-10-03 17:43:47]

Please take a look.

[aram, 2012-10-03 17:46:57]

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

https://codereview.appspot.com/6595064/diff/1/state/service_test.go#newcode345
state/service_test.go:345: type unitSlice []*state.Unit
This is not new code, it's just old code that rietveld mistakenly believes it
was moved. This will go away after integrating
https://codereview.appspot.com/6573064/

[niemeyer, 2012-10-03 19:09:13]

https://codereview.appspot.com/6595064/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6595064/diff/1/state/service.go#newcode27
state/service.go:27: Units      []string
It's quite unfortunate, but we can't have that. A single service may have dozens
of thousands of units, so this watcher will have to operate on the units
collection itself. Luckily, we can tell a unit pertains to a service by its name
as we were doing before.

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

https://codereview.appspot.com/6595064/diff/1/state/watcher.go#newcode375
state/watcher.go:375: // units and units which have changed their lifecycle, up
to when they are
Comment looks good. Would just prefer to see the last sentence stated as we have
elsewhere, for clarity:

// ... changed their lifecycle.
// After a unit is found to be Dead, no further event will include it.

https://codereview.appspot.com/6595064/diff/1/state/watcher.go#newcode412
state/watcher.go:412: return w.updateService(changes)
Let's inline this as well.

https://codereview.appspot.com/6595064/diff/1/state/watcher.go#newcode432
state/watcher.go:432: func (w *ServiceUnitsWatcher) merge(pending []string, name
string) (changes []string, err error) {
Structure is generally pretty good. I suspect we don't need to watch the service
itself due to the mentioned issue, and some details below need changing in the
same way that we've changed in the previous branch reviewed.

[aram, 2012-10-08 20:08:02]

Please take a look.

[niemeyer, 2012-10-09 18:01:59]

LGTM. A few last points for your consideration:

https://codereview.appspot.com/6595064/diff/8001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/6595064/diff/8001/state/service_test.go#newcod...
state/service_test.go:361: {
We can replace all these with "}, {". Some more space with no downsides.

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode419
state/watcher.go:419: return append(pending, name), nil
Do we need to check if it's already in pending here?

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode441
state/watcher.go:441: err = w.st.units.Find(D{{"service",
w.service.doc.Name}}).Select(D{{"_id", 1}, {"Life", 1}}).All(&docs)
s/Life/life/, I believe. Do any tests break if you set this to "foo"?

Also, Iter() would be better here rather than All. It's cheaper and simpler.

[niemeyer, 2012-10-09 21:22:48]

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode409
state/watcher.go:409: func (w *ServiceUnitsWatcher) merge(pending []string, name
string) (changes []string, err error) {
I just realized something: if name is in pending, we can return from here,
without ever querying the database again. This will save unnecessary re-queries
while we're waiting for an event to be consumed.

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode419
state/watcher.go:419: return append(pending, name), nil
On 2012/10/09 18:01:59, niemeyer wrote:
> Do we need to check if it's already in pending here?

And then the answer to this becomes "no".

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode431
state/watcher.go:431: for _, v := range pending {
And we don't need to check here.

[niemeyer, 2012-10-09 21:23:14]

LGTM given the above straightforward change still.

[aram, 2012-10-10 12:15:35]

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode409
state/watcher.go:409: func (w *ServiceUnitsWatcher) merge(pending []string, name
string) (changes []string, err error) {
> I just realized something: if name is in pending, we can return
> from here,without ever querying the database again. This will
> save unnecessary re-queries while we're waiting for an event to
> be consumed.

We need to query the database to update the knowledge, otherwise we would not be
able to assess the need for merging in the future.

[aram, 2012-10-10 14:34:25]

https://codereview.appspot.com/6595064/diff/8001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/6595064/diff/8001/state/service_test.go#newcod...
state/service_test.go:361: {
On 2012/10/09 18:01:59, niemeyer wrote:
> We can replace all these with "}, {". Some more space with no downsides.

Done.

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode419
state/watcher.go:419: return append(pending, name), nil
On 2012/10/09 18:01:59, niemeyer wrote:
> Do we need to check if it's already in pending here?

Done.

https://codereview.appspot.com/6595064/diff/8001/state/watcher.go#newcode441
state/watcher.go:441: err = w.st.units.Find(D{{"service",
w.service.doc.Name}}).Select(D{{"_id", 1}, {"Life", 1}}).All(&docs)
> s/Life/life/, I believe. Do any tests break if you set this to "foo"?

No. Fixed and added a test that breaks without the fix.

[aram, 2012-10-10 14:34:52]

*** Submitted:

state: update ServiceUnitsWatcher

Add lifecycle support to the ServiceUnitsWatcher and make it a new style
watcher that only returns ids.

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