state: drop kill and Die method names

state: drop kill and Die method names

s/Kill/EnsureDying/
s/Die/EnsureDead/

The above methods are now implemented in terms of ensureDying and ensureDead
funcs; ensureDead allows you to pass a list of additional txn.Ops that can
block entity death by including Asserts.

https://code.launchpad.net/~fwereade/juju-core/rename-kill-die/+merge/125761

(do not edit description out of merge proposal)

[fwereade, 2012-09-21 16:26:50]

Please take a look.

[aram, 2012-09-21 16:34:38]

LGTM

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

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode201
state/service.go:201: s.doc.UnitCount += 1
Refresh perhaps?

[aram, 2012-09-21 16:37:28]

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

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode201
state/service.go:201: s.doc.UnitCount += 1
On 2012/09/21 16:34:38, aram wrote:
> Refresh perhaps?

Better to remove completely from memory.

[fwereade, 2012-09-21 16:41:42]

Please take a look.

[TheMue, 2012-09-21 16:47:32]

LGTM.

https://codereview.appspot.com/6553051/diff/5001/state/tools_test.go
File state/tools_test.go (right):

https://codereview.appspot.com/6553051/diff/5001/state/tools_test.go#newcode15
state/tools_test.go:15: Life() state.Life
Thought the lifer interface could be embedded here, but it's a different test.

[niemeyer, 2012-09-21 16:57:37]

https://codereview.appspot.com/6553051/diff/1/state/assign_test.go
File state/assign_test.go (left):

https://codereview.appspot.com/6553051/diff/1/state/assign_test.go#oldcode347
state/assign_test.go:347: func (s *AssignSuite)
TestAssignUnitToUnusedMachineWithRemovedService(c *C) {
Why is this going away?

https://codereview.appspot.com/6553051/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6553051/diff/1/state/life.go#newcode53
state/life.go:53: return fmt.Errorf("cannot set life to Dying for %s %q: %v",
desc, id, err)
I suggest we use a slightly more user-oriented message here:

"cannot start termination of %s %#v: %v"

(please note the %#v there.. %q is wrong for machine)

https://codereview.appspot.com/6553051/diff/1/state/life.go#newcode62
state/life.go:62: defer trivial.ErrorContextf(&err, "cannot set life to Dead for
%s %q", desc, id)
"cannot finish termination of %s %#v"

https://codereview.appspot.com/6553051/diff/1/state/life.go#newcode73
state/life.go:73: var doc struct{ Life Life }
Just one Life is enough. (!)

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

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode68
state/service.go:68: assertOps, "service still has units",
"service still has non-terminated units"

Also, as a nitpick, we generally break method calls as:

    err := foo(a, b, c,
            d, e, f)

(or don't break at all)

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode201
state/service.go:201: s.doc.UnitCount += 1
On 2012/09/21 16:37:29, aram wrote:
> On 2012/09/21 16:34:38, aram wrote:
> > Refresh perhaps?
> 
> Better to remove completely from memory.

+1

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode287
state/service.go:287: log.Printf("BLAM %#v", err)
d

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode288
state/service.go:288: // If aborted, the unit is either dead or recreated.
Can we please put a safe-guard here until we learn that the logic above is
right:

// TODO Remove this once we know the logic is right:
if c, err := s.st.units.FindId(u.doc.Name).Count(); err != nil {
    return err
} else if c > 0 {
    return fmt.Errorf("cannot remove unit; something smells bad")
}

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode291
state/service.go:291: s.doc.UnitCount -= 1
d

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode334
state/service.go:334: func (s *Service) UnitCount() int {
d

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

https://codereview.appspot.com/6553051/diff/1/state/service_test.go#newcode249
state/service_test.go:249: // ...and when those units are Dying or Dead...
Doesn't that cover also what's done above?

https://codereview.appspot.com/6553051/diff/1/state/service_test.go#newcode302
state/service_test.go:302: c.Assert(err, IsNil)
One more time here?

err = s.service.EnsureDead()
c.Assert(err, ErrorMatches, `cannot set life to Dead for service "mysql":
service still has units`)

https://codereview.appspot.com/6553051/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6553051/diff/1/state/unit.go#newcode152
state/unit.go:152: // EnsureDying sets the unit lifecycle to Dying if it is
Alive.
So much more clear.

[fwereade, 2012-09-21 17:43:48]

Please take a look.

https://codereview.appspot.com/6553051/diff/1/state/assign_test.go
File state/assign_test.go (left):

https://codereview.appspot.com/6553051/diff/1/state/assign_test.go#oldcode347
state/assign_test.go:347: func (s *AssignSuite)
TestAssignUnitToUnusedMachineWithRemovedService(c *C) {
On 2012/09/21 16:57:37, niemeyer wrote:
> Why is this going away?

Because I misunderstood the intent of the test. Reinstated.

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

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode68
state/service.go:68: assertOps, "service still has units",
On 2012/09/21 16:57:37, niemeyer wrote:
> "service still has non-terminated units"
> 
> Also, as a nitpick, we generally break method calls as:
> 
>     err := foo(a, b, c,
>             d, e, f)
> 
> (or don't break at all)

Done (but agreed live that message is OK)

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode201
state/service.go:201: s.doc.UnitCount += 1
On 2012/09/21 16:57:37, niemeyer wrote:
> On 2012/09/21 16:37:29, aram wrote:
> > On 2012/09/21 16:34:38, aram wrote:
> > > Refresh perhaps?
> > 
> > Better to remove completely from memory.
> 
> +1

Done.

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode287
state/service.go:287: log.Printf("BLAM %#v", err)
On 2012/09/21 16:57:37, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode288
state/service.go:288: // If aborted, the unit is either dead or recreated.
On 2012/09/21 16:57:37, niemeyer wrote:
> Can we please put a safe-guard here until we learn that the logic above is
> right:
> 
> // TODO Remove this once we know the logic is right:
> if c, err := s.st.units.FindId(u.doc.Name).Count(); err != nil {
>     return err
> } else if c > 0 {
>     return fmt.Errorf("cannot remove unit; something smells bad")
> }

Done.

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode291
state/service.go:291: s.doc.UnitCount -= 1
On 2012/09/21 16:57:37, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode334
state/service.go:334: func (s *Service) UnitCount() int {
On 2012/09/21 16:57:37, niemeyer wrote:
> d

Done.

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

https://codereview.appspot.com/6553051/diff/1/state/service_test.go#newcode249
state/service_test.go:249: // ...and when those units are Dying or Dead...
On 2012/09/21 16:57:37, niemeyer wrote:
> Doesn't that cover also what's done above?

No :).

https://codereview.appspot.com/6553051/diff/1/state/service_test.go#newcode302
state/service_test.go:302: c.Assert(err, IsNil)
On 2012/09/21 16:57:37, niemeyer wrote:
> One more time here?
> 
> err = s.service.EnsureDead()
> c.Assert(err, ErrorMatches, `cannot set life to Dead for service "mysql":
> service still has units`)

Done.

[niemeyer, 2012-09-21 17:57:39]

LGTM

[fwereade, 2012-09-24 09:33:49]

*** Submitted:

state: drop kill and Die method names

s/Kill/EnsureDying/
s/Die/EnsureDead/

The above methods are now implemented in terms of ensureDying and ensureDead
funcs; ensureDead allows you to pass a list of additional txn.Ops that can
block entity death by including Asserts.

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

https://codereview.appspot.com/6553051/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6553051/diff/1/state/life.go#newcode53
state/life.go:53: return fmt.Errorf("cannot set life to Dying for %s %q: %v",
desc, id, err)
On 2012/09/21 16:57:37, niemeyer wrote:
> I suggest we use a slightly more user-oriented message here:
> 
> "cannot start termination of %s %#v: %v"
> 
> (please note the %#v there.. %q is wrong for machine)

Done.

https://codereview.appspot.com/6553051/diff/1/state/life.go#newcode62
state/life.go:62: defer trivial.ErrorContextf(&err, "cannot set life to Dead for
%s %q", desc, id)
On 2012/09/21 16:57:37, niemeyer wrote:
> "cannot finish termination of %s %#v"

Done.

https://codereview.appspot.com/6553051/diff/1/state/life.go#newcode73
state/life.go:73: var doc struct{ Life Life }
On 2012/09/21 16:57:37, niemeyer wrote:
> Just one Life is enough. (!)

Done.

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

https://codereview.appspot.com/6553051/diff/1/state/service.go#newcode27
state/service.go:27: UnitCount  int
Reinstated per live discussion.