Code review comment for lp:~rogpeppe/pyjuju/go-state-units-under-service

submitted in branch from juju-core

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
state/machine.go:88: // keyId returns the id corresponding to the given
machine or unit id.
On 2012/06/06 20:15:09, niemeyer wrote:
> s/keyId/keySequence/ or s/keyId/keySeq/

> 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).

Done.

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#newcode84
state/service.go:84: kprefix = kprefix[:strings.LastIndex(kprefix,
"-")+1]
On 2012/06/06 20:15:09, niemeyer wrote:
> 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 +
> "-".

it's not *quite* as simple as that (makeUnitKey strips off the initial
"service-" prefix) but done anyway.

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode120
state/service.go:120: return zkRemoveTree(s.st.zk, unit.zkPath())
On 2012/06/04 07:29:19, fwereade wrote:
> Not relevant to this CL... but do we actually want to do this? The
unit agent
> will surely still be running; I'm not sure it'll handle this all that
well...

that's an interesting point (although, as you say, not relevant to this
CL). what *should* we do here? wait until the unit agent has gone away?
leave the directory around and GC it later? or perhaps the best approach
*is* to delete the directory and let the unit agent detect that.

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
state/unit.go:77: func mkUnitKey(serviceKey string, unitId int) string {
On 2012/06/06 20:15:09, niemeyer wrote:
> 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 two
> chars. Even more when we have serviceKeyForUnitKey below. :-)

done. old C proclivities creeping in there... :-)

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/06 20:15:09, niemeyer wrote:
> 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.

oops. done.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode518
state/unit.go:518: func parseUnitName(name string) (serviceName string,
serviceId int, err error) {
On 2012/06/06 20:15:09, niemeyer wrote:
> s/serviceId/serviceSeq/

Done.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode523
state/unit.go:523: id, err := strconv.ParseInt(parts[1], 10, 0)
On 2012/06/06 20:15:09, niemeyer wrote:
> s/id/seq/

Done.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode527
state/unit.go:527: return parts[0], int(id), nil
On 2012/06/06 20:15:09, niemeyer wrote:
> s/id/seq/

Done.

https://codereview.appspot.com/6247066/