Code review comment for lp:~themue/juju-core/024-cleanup-watcher

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

A few comments.

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

https://codereview.appspot.com/10078043/diff/6001/state/state.go#newcode1079
state/state.go:1079: TxnRevno int64 `bson:"txn-revno"`
You don't need to define this, it's automatically available.

https://codereview.appspot.com/10078043/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10078043/diff/6001/state/state_test.go#newcode1362
state/state_test.go:1362: case _, ok := <-cw.Changes():
Please add c.Assert(ok, Equals, true), otherwise this is not a change
notification.

https://codereview.appspot.com/10078043/diff/6001/state/state_test.go#newcode1390
state/state_test.go:1390: _, err = s.State.AddService("varnish",
s.AddTestingCharm(c, "varnish"))
do you really need to do this twice? surely only one relation followed
by destroy should be sufficient, but ymmv

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

https://codereview.appspot.com/10078043/diff/6001/state/watcher.go#newcode1256
state/watcher.go:1256: in chan watcher.Change
in chan doesn't need to be a field of the watcher, you can create it
locally inside loop()

https://codereview.appspot.com/10078043/diff/6001/state/watcher.go#newcode1288
state/watcher.go:1288: out := w.out
is there always an initial event that makes this line necessary? istm
this will always cause a notification to be sent, even when there's
nothing coming on the in chan.

https://codereview.appspot.com/10078043/

« Back to merge proposal