mstate: changed lifecycle implementation in unit

mstate: changed lifecycle implementation in unit

Unit now has Life(), Kill() and Die() and uses the utility
function ensureLife(). The test for removing a unit has
been changed.

https://code.launchpad.net/~themue/juju-core/go-mstate-unit-lifecycle/+merge/120744

(do not edit description out of merge proposal)

[TheMue, 2012-08-22 10:37:12]

Please take a look.

[aram, 2012-08-22 11:29:57]

Thanks for this. I don't see any lifecycle tests though. Could you add some? A
test needs to be like TestLifecycleStateChanges, please and use the stateChanges
table for the tests.

https://codereview.appspot.com/6476044/diff/1/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/service.go#newcode174
mstate/service.go:174: {"_id", u.doc.Name},
Why did you remove the check for service?

https://codereview.appspot.com/6476044/diff/1/mstate/service.go#newcode179
mstate/service.go:179: return err
Could you please put the error here instead of using ErrorContextf?

https://codereview.appspot.com/6476044/diff/1/mstate/service_test.go
File mstate/service_test.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/service_test.go#newcode185
mstate/service_test.go:185: err = unit.Die()
It's fine to call unit.Die() directly, without a Kill().

https://codereview.appspot.com/6476044/diff/1/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/state.go#newcode43
mstate/state.go:43: func ensureLife(id interface{}, coll *mgo.Collection, descr
string, life Life) error {
It's a pity we have to use interface{} here. In the long run we should probably
change integer ids to string ids.

https://codereview.appspot.com/6476044/diff/1/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/unit.go#newcode104
mstate/unit.go:104: if err := ensureLife(u.doc.Name, u.st.units, "unit", Dying);
err != nil {
Could you please put this on two separate lines? Thanks.

https://codereview.appspot.com/6476044/diff/1/mstate/unit.go#newcode114
mstate/unit.go:114: if err := ensureLife(u.doc.Name, u.st.units, "unit", Dead);
err != nil {
Ditto.

[TheMue, 2012-08-22 11:49:33]

Please take a look.

https://codereview.appspot.com/6476044/diff/1/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/service.go#newcode174
mstate/service.go:174: {"_id", u.doc.Name},
On 2012/08/22 11:29:58, aram wrote:
> Why did you remove the check for service?

Done.

https://codereview.appspot.com/6476044/diff/1/mstate/service.go#newcode179
mstate/service.go:179: return err
On 2012/08/22 11:29:58, aram wrote:
> Could you please put the error here instead of using ErrorContextf?

Done.

https://codereview.appspot.com/6476044/diff/1/mstate/service_test.go
File mstate/service_test.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/service_test.go#newcode185
mstate/service_test.go:185: err = unit.Die()
On 2012/08/22 11:29:58, aram wrote:
> It's fine to call unit.Die() directly, without a Kill().

Is that OK? IMHO it shouldn't be. There my watchers be interested when an entity
is marked as Dying but they never will get it that way.

https://codereview.appspot.com/6476044/diff/1/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/state.go#newcode43
mstate/state.go:43: func ensureLife(id interface{}, coll *mgo.Collection, descr
string, life Life) error {
On 2012/08/22 11:29:58, aram wrote:
> It's a pity we have to use interface{} here. In the long run we should
probably
> change integer ids to string ids.

Yes, and you have seen that rog is interested in string machine ids too. So
let's discuss it with niemeyer.

https://codereview.appspot.com/6476044/diff/1/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/unit.go#newcode104
mstate/unit.go:104: if err := ensureLife(u.doc.Name, u.st.units, "unit", Dying);
err != nil {
On 2012/08/22 11:29:58, aram wrote:
> Could you please put this on two separate lines? Thanks.

As discussed I still like the one-liner but changing it is ok for me, too.

https://codereview.appspot.com/6476044/diff/1/mstate/unit.go#newcode114
mstate/unit.go:114: if err := ensureLife(u.doc.Name, u.st.units, "unit", Dead);
err != nil {
On 2012/08/22 11:29:58, aram wrote:
> Ditto.

Done.

[aram, 2012-08-22 11:57:22]

Thanks, I still want a comprehensive test that uses the stateChanges table, like
TestLifecycleStateChanges does.

https://codereview.appspot.com/6476044/diff/1/mstate/service_test.go
File mstate/service_test.go (right):

https://codereview.appspot.com/6476044/diff/1/mstate/service_test.go#newcode185
mstate/service_test.go:185: err = unit.Die()
> Is that OK? IMHO it shouldn't be. There my watchers be interested
> when an entity is marked as Dying but they never will get it
> that way.

I was of the same opinion, but after debating it to death with
niemeyer about it I made this way. The idea is that you do it
when you know nobody cares about dying, e.g. you just call x.Die()
so *YOU* can call x.Remove().

[aram, 2012-08-22 14:49:50]

> I still want a comprehensive test that uses the stateChanges table, like
> TestLifecycleStateChanges does.

You've done it in a subsequent review, so LGTM.

[niemeyer, 2012-08-23 22:49:15]

There are missing tests, but I've seen Aram commenting that you're doing them in
a follow up. If so, LGTM.

https://codereview.appspot.com/6476044/diff/3002/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/service.go#newcode169
mstate/service.go:169: panic(fmt.Errorf("unit %q is not dead", u))
There's no reason to explode the world if there's an error like this. It's
relatively easy to make that mistake, and it will not improve the situation
further.

https://codereview.appspot.com/6476044/diff/3002/mstate/service.go#newcode178
mstate/service.go:178: return fmt.Errorf("cannot remove unit %q: %v",
u.doc.Name, err)
s/u.doc.Name/u/

The unit should have a proper String.

https://codereview.appspot.com/6476044/diff/3002/mstate/service_test.go
File mstate/service_test.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/service_test.go#newco...
mstate/service_test.go:183: err = unit.Kill()
We can call Die directly, I believe.

https://codereview.appspot.com/6476044/diff/3002/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/state.go#newcode59
mstate/state.go:59: return fmt.Errorf("cannot set life to %s for %s %q: %v",
life, descr, id, err)
s/life to %s/life to %q/

https://codereview.appspot.com/6476044/diff/3002/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/unit.go#newcode96
mstate/unit.go:96: // Life return the unit lifecycle.
// Life returns whether the unit is Alive, Dying or Dead.

[TheMue, 2012-08-24 08:56:17]

*** Submitted:

mstate: changed lifecycle implementation in unit

Unit now has Life(), Kill() and Die() and uses the utility
function ensureLife(). The test for removing a unit has
been changed.

R=aram, niemeyer
CC=
https://codereview.appspot.com/6476044

https://codereview.appspot.com/6476044/diff/3002/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/service.go#newcode169
mstate/service.go:169: panic(fmt.Errorf("unit %q is not dead", u))
On 2012/08/23 22:49:15, niemeyer wrote:
> There's no reason to explode the world if there's an error like this. It's
> relatively easy to make that mistake, and it will not improve the situation
> further.

Done.

https://codereview.appspot.com/6476044/diff/3002/mstate/service.go#newcode178
mstate/service.go:178: return fmt.Errorf("cannot remove unit %q: %v",
u.doc.Name, err)
On 2012/08/23 22:49:15, niemeyer wrote:
> s/u.doc.Name/u/
> 
> The unit should have a proper String.

Done.

https://codereview.appspot.com/6476044/diff/3002/mstate/service_test.go
File mstate/service_test.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/service_test.go#newco...
mstate/service_test.go:183: err = unit.Kill()
On 2012/08/23 22:49:15, niemeyer wrote:
> We can call Die directly, I believe.

Yes, discussed it already with Aram.

https://codereview.appspot.com/6476044/diff/3002/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/state.go#newcode59
mstate/state.go:59: return fmt.Errorf("cannot set life to %s for %s %q: %v",
life, descr, id, err)
On 2012/08/23 22:49:15, niemeyer wrote:
> s/life to %s/life to %q/

Done.

https://codereview.appspot.com/6476044/diff/3002/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6476044/diff/3002/mstate/unit.go#newcode96
mstate/unit.go:96: // Life return the unit lifecycle.
On 2012/08/23 22:49:15, niemeyer wrote:
> // Life returns whether the unit is Alive, Dying or Dead.

Done.