Code review comment for lp:~johnweldon4/juju-core/action
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Many comments, because I like the sound of my own typing, but very
little actionable. I'd appreciate your thoughts but this basically LGTM.
Fix any trivials and let me know when it's ready to merge, and I'll
approve it.
https:/
File state/unit.go (right):
https:/
state/unit.go:1325: if notDead, err := isNotDead(
u.doc.Name); err != nil {
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.
https:/
state/unit.go:1331: sel := bson.D{{"_id", newActionID}}
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
be pretty easy to diagnose from there.
https:/
state/unit.go:1340: ops := []txn.Op{{
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.
https:/
state/unit.go:1351: return newActionID, err
returning the id is not "normal" for this package, but IMO it's a good
idea. thanks.
https:/
File state/unit_test.go (right):
https:/
state/unit_
id prefix, + sequence)
Add a note that this is temporary, and we shouldn't really leak the
actual _id.
https:/
state/unit_
s/unitDead/
https:/
state/unit_
'contention' into the transaction
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.)
https:/