Code review comment for lp:~johnweldon4/juju-core/action
Revision history for this message
| John Weldon (johnweldon4) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Addressed the few changes needed. Couple questions I'll ask about in
#juju-dev
https:/
File state/unit.go (right):
https:/
state/unit.go:1325: if notDead, err := isNotDead(
u.doc.Name); err != nil {
On 2014/05/21 07:39:48, fwereade wrote:
> This one is interesting -- it's a not-strictly-
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:/
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 ErrExcessiveCon
easy to
> diagnose from there.
Done.
https:/
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:/
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:/
File state/unit_test.go (right):
https:/
state/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:/
state/unit_
On 2014/05/21 07:39:48, fwereade wrote:
> s/unitDead/
Done.
https:/
state/unit_
'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:/