Very nice change. LGTM, assuming you're happy with the adaptations
below. Otherwise, please reply accordingly and repropose.
Please note you'll have to propose+submit this to the new juju-core
project, since the old trunk is now gone (sorry!). You can do this by
creating a new branch from the new project as you'd usually do, merging
from this work onto it, and then just proposing as usual.
This is not an identifier. "unit-0000001" is the identifier, and we use
the term "key" there, after much debate, precisely to avoid the
confusion with the idea of an "id" which is public. We can easily avoid
the confusion still by calling the sequence number what it really is (it
comes from the idea of sequence from ZooKeeper).
As a special case, we do call the *machine* sequence number an
identifier, because that's how we handle it across the board
(machine.Id(), etc).
https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode84
state/service.go:84: kprefix = kprefix[:strings.LastIndex(kprefix,
"-")+1]
There's no reason to go over the trouble here. It's already assuming
knowledge about how makeUnitKey works (who said it could skip 'til the
last dash like that?) so it may as well be a lot more clear and say
"/units/unit-" + s.key + "-".
Very nice change. LGTM, assuming you're happy with the adaptations
below. Otherwise, please reply accordingly and repropose.
Please note you'll have to propose+submit this to the new juju-core
project, since the old trunk is now gone (sorry!). You can do this by
creating a new branch from the new project as you'd usually do, merging
from this work onto it, and then just proposing as usual.
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/machine. go
File state/machine.go (right):
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/machine. go#newcode88 go:88: // keyId returns the id corresponding to the given keySequence/ or s/keyId/keySeq/
state/machine.
machine or unit id.
s/keyId/
This is not an identifier. "unit-0000001" is the identifier, and we use
the term "key" there, after much debate, precisely to avoid the
confusion with the idea of an "id" which is public. We can easily avoid
the confusion still by calling the sequence number what it really is (it
comes from the idea of sequence from ZooKeeper).
As a special case, we do call the *machine* sequence number an
identifier, because that's how we handle it across the board
(machine.Id(), etc).
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/service. go
File state/service.go (right):
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/service. go#newcode13 go:13: pathPkg "path"
state/service.
stdpath
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/service. go#newcode84 go:84: kprefix = kprefix[ :strings. LastIndex( kprefix,
state/service.
"-")+1]
There's no reason to go over the trouble here. It's already assuming
knowledge about how makeUnitKey works (who said it could skip 'til the
last dash like that?) so it may as well be a lot more clear and say
"/units/unit-" + s.key + "-".
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/unit. go
File state/unit.go (right):
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/unit. go#newcode77 serviceKey string, unitId int) string {
state/unit.go:77: func mkUnitKey(
On 2012/06/04 07:29:19, fwereade wrote:
> What does "mk" signify?
I assume "make", but I'd also prefer the word itself rather than saving itKey below. :-)
two chars. Even more when we have serviceKeyForUn
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/unit. go#newcode86
state/unit.go:86: return "", fmt.Errorf("invalid unit key %q (1) %s",
unitKey, debug.Callers(0, 10))
On 2012/06/04 07:29:19, fwereade wrote:
> Is this the proposed implementation or an oversight?
Yeah, looks like an oversight. We need something closer to the error
message of makeUnitKey.
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/unit. go#newcode518 serviceSeq/
state/unit.go:518: func parseUnitName(name string) (serviceName string,
serviceId int, err error) {
s/serviceId/
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/unit. go#newcode523 ParseInt( parts[1] , 10, 0)
state/unit.go:523: id, err := strconv.
s/id/seq/
https:/ /codereview. appspot. com/6247066/ diff/7001/ state/unit. go#newcode527
state/unit.go:527: return parts[0], int(id), nil
s/id/seq/
https:/ /codereview. appspot. com/6247066/