Addressed the few changes needed.  Couple questions I'll ask about in
#juju-dev



https://codereview.appspot.com/98260043/diff/40001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1325
state/unit.go:1325: if notDead, err := isNotDead(u.st.units,
u.doc.Name); err != nil {
On 2014/05/21 07:39:48, fwereade wrote:
> This one is interesting -- it's a not-strictly-necessary DB hit before
running
> the txn, and I'd ordinarily shy away from that in favour of checking
u.doc.Life
> -- but let's see how it goes. I suspect we'll need to go with the
other approach
> in the end, as we start to care about things like charm versions, but
we can
> wait and see if I'm right about that.

How is this different than doing a clone of the unit as you initially
suggested.  I went this route because it seemed more direct and I
thought the db hit would be comparable, if not slightly lighter than a
full clone?

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1331
state/unit.go:1331: sel := bson.D{{"_id", newActionID}}
On 2014/05/21 07:39:48, fwereade wrote:
> This block is almost redundant, because we should be able to depend on
the
> uniqueness of sequence IDs (even in the case of a new unit being
created with
> the same name as an old one (which is itself a bug (although it only
happens
> when a service is destroyed and recreated))).

> If it didn't involve an extra DB hit I'd have no problem with it; but
as it is
> I'm gently leaning towards dropping it. If there's a problem here
we'll fall out
> to ErrExcessiveContention anyway, and it should actually be pretty
easy to
> diagnose from there.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1340
state/unit.go:1340: ops := []txn.Op{{
On 2014/05/21 07:39:48, fwereade wrote:
> As discussed yesterday, in this particular case the ops don't have to
be rebuilt
> each time, but it's probably good to keep the familiar structure.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1351
state/unit.go:1351: return newActionID, err
On 2014/05/21 07:39:48, fwereade wrote:
> returning the id is not "normal" for this package, but IMO it's a good
idea.
> thanks.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1304
state/unit_test.go:1304: // verify action Id() is of expected form (unit
id prefix, + sequence)
On 2014/05/21 07:39:48, fwereade wrote:
> Add a note that this is temporary, and we shouldn't really leak the
actual _id.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1335
state/unit_test.go:1335: unitDead := state.TransactionHook{
On 2014/05/21 07:39:48, fwereade wrote:
> s/unitDead/killUnit/ perhaps?

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1348
state/unit_test.go:1348: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
On 2014/05/21 07:39:48, fwereade wrote:
> Haha. I've thought of a way, and I kinda endorse you doing it, so long
as you
> clearly comment that it'll stop working when lp:1174610 is fixed. In
the Before
> hook, you get st.Unit(name) and make it Dead; in the After hook, you
remove the
> unit and the service, and create a new service with the same name.
 From the POV
> of that method the unit will appear to be resurrected.

> But that's also dirty as sin, and will break when we fix other stuff,
so I'm
> reasonably comfortable skipping it. (The other possibility, which is
*maybe*
> cleaner, is to figure out the action doc id and create/remove a doc
with that
> name in the before/after hooks. But figuring out the id without dirt
may be
> tricky -- expose sequence() in export_test, and subtract 1? Could
work, but
> still depends on knowing _id structure; don't really love either.)

1. 'dirty as sin', and
2. lots of work

Hmm, since you gave me an out I'd be happy to skip :)

https://codereview.appspot.com/98260043/