state: update ServiceRelationsWatcher

state: update ServiceRelationsWatcher

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

https://code.launchpad.net/~aramh/juju-core/102-state-watchers-service-relations5/+merge/128577

(do not edit description out of merge proposal)

[aram, 2012-10-08 20:09:23]

Please take a look.

[fwereade, 2012-10-09 07:17:51]

Aside from uncertainty about the testing, LGTM.

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

https://codereview.appspot.com/6631046/diff/1/state/service_test.go#newcode558
state/service_test.go:558: continue
Doesn't this deliberately avoid testing the important bit of any initial event
with actual content (namely that it comes all at once)? I see how it's useful
for subsequent tests, but...

[aram, 2012-10-09 10:57:31]

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

https://codereview.appspot.com/6631046/diff/1/state/service_test.go#newcode558
state/service_test.go:558: continue
Good catch, thanks.

[aram, 2012-10-09 10:57:43]

Please take a look.

[niemeyer, 2012-10-09 18:57:39]

LGTM assuming the following comments, that all tests pass, and that William is
happy with it.

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode537
state/watcher.go:537: err = w.st.relations.Find(D{{"endpoints.servicename",
w.service.doc.Name}}).Select(D{{"_id", 1}, {"id", 1}, {"life", 1}}).All(&docs)
Iter would be better here.

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode538
state/watcher.go:538: if err == mgo.ErrNotFound {
All and Iter never return ErrNotFound.

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode561
state/watcher.go:561: return append(pending, old.Id), nil
Should this be checking for duplicates too?

[niemeyer, 2012-10-09 21:25:00]

LGTM considering the previous review still, plus the following point:

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode551
state/watcher.go:551: func (w *ServiceRelationsWatcher) merge(pending []int, key
string) (new []int, 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/6631046/diff/6001/state/watcher.go#newcode561
state/watcher.go:561: return append(pending, old.Id), nil
On 2012/10/09 18:57:40, niemeyer wrote:
> Should this be checking for duplicates too?

And then the answer to this becomes "no".

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode570
state/watcher.go:570: for _, v := range pending {
And we don't need to check here.

[fwereade, 2012-10-10 08:12:29]

LGTM with niemeyer's suggestions

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode497
state/watcher.go:497: // relation is found to be Dead, no further event will
include it.
Just a suggestion: s/found to be/reported to be/ ?

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode551
state/watcher.go:551: func (w *ServiceRelationsWatcher) merge(pending []int, key
string) (new []int, err error) {
On 2012/10/09 21:25:00, niemeyer wrote:
> 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.

+1

[niemeyer, 2012-10-10 13:08:01]

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6631046/diff/6001/state/watcher.go#newcode497
state/watcher.go:497: // relation is found to be Dead, no further event will
include it.
On 2012/10/10 08:12:29, fwereade wrote:
> Just a suggestion: s/found to be/reported to be/ ?

SGTM, but if we change this all instances should be changed as well (this is
somewhat of a template we use in all watchers).

[aram, 2012-10-10 13:09:00]

> if we change this all instances should be changed as well (this is
> somewhat of a template we use in all watchers).

SGTM

[aram, 2012-10-10 15:00:42]

*** Submitted:

state: update ServiceRelationsWatcher

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

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6631046