mstate: add service units watcher

mstate: add service units watcher



https://code.launchpad.net/~aramh/juju-core/75-mstate-watchers-units-nwo/+merge/124867

(do not edit description out of merge proposal)

[aram, 2012-09-18 09:36:19]

Please take a look.

[niemeyer, 2012-09-18 10:24:14]

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

https://codereview.appspot.com/6524044/diff/1/mstate/watcher.go#newcode408
mstate/watcher.go:408: func (w *ServiceUnitsWatcher) mergeChange(changes
*ServiceUnitsChange, ch watcher.Change) (err error) {
We should check here that the change is for a unit we care about, by comparing
its name with a "service/" prefix (let's keep that prefix in w to prevent
computing it every single time). Otherwise the logic below will execute for
every single unit in the system.

https://codereview.appspot.com/6524044/diff/1/mstate/watcher.go#newcode417
mstate/watcher.go:417: err = w.st.units.Find(D{
Given the above, we can use FindId here.

[aram, 2012-09-18 15:00:49]

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

https://codereview.appspot.com/6524044/diff/1/mstate/watcher.go#newcode408
mstate/watcher.go:408: func (w *ServiceUnitsWatcher) mergeChange(changes
*ServiceUnitsChange, ch watcher.Change) (err error) {
On 2012/09/18 10:24:15, niemeyer wrote:
> We should check here that the change is for a unit we care about, by comparing
> its name with a "service/" prefix (let's keep that prefix in w to prevent
> computing it every single time). Otherwise the logic below will execute for
> every single unit in the system.

Done.

https://codereview.appspot.com/6524044/diff/1/mstate/watcher.go#newcode417
mstate/watcher.go:417: err = w.st.units.Find(D{
On 2012/09/18 10:24:15, niemeyer wrote:
> Given the above, we can use FindId here.

Done.

[aram, 2012-09-18 15:07:24]

Please take a look.

[niemeyer, 2012-09-18 15:17:28]

https://codereview.appspot.com/6524044/diff/5001/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6524044/diff/5001/mstate/watcher.go#newcode413
mstate/watcher.go:413: if name[:len(w.service.doc.Name)] != w.service.doc.Name {
What if name is "foos/1" and doc.Name is "foo/1"?

Please use:

    if strings.HasPrefix(name, w.prefix)

where w.prefix is "foo/".

[aram, 2012-09-18 15:25:44]

Please take a look.

[aram, 2012-09-18 15:38:49]

Please take a look.

[niemeyer, 2012-09-18 15:43:08]

LGTM given the previous suggestion is applied:

https://codereview.appspot.com/6524044/diff/8003/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6524044/diff/8003/mstate/watcher.go#newcode411
mstate/watcher.go:411: if !strings.HasPrefix(name, w.service.doc.Name+"/") {
Please save the value as w.prefix as suggested. No point in recomputing this all
the time.

[aram, 2012-09-18 15:47:03]

*** Submitted:

mstate: add service units watcher

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