Code review comment for lp://staging/~tasdomas/juju-core/ensure-availability-output
Revision history for this message
| Roger Peppe (rogpeppe) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
513534c
demo site
(Get the code!)
This looks fine for what it's trying to do, thanks, but I think we
should get state.EnsureAva
rather than inferring it by looking at the resulting state servers. And
one more suggestion below.
https:/
File state/state.go (right):
https:/
state/state.go:380: // MachineIdLessThan returns true if id1 < id2,
false otherwise.
Go-conventional phrasing for this would be:
// MachineIdLessThan reports whether id1 is less than id2.
https:/
state/state.go:385: func MachineIdLessTh
On 2014/06/02 09:50:03, fwereade wrote:
> If you're exporting this, I think it would probably live most happily
in the
> names package.
+1
BTW, it seems to me that this is somewhat overkill for what we're
actually using it for, which is to sort state server machines which are
never containerised in fact.
That said, since this is written, we might as well keep it - then
again, I wonder if we should just use exactly the same logic that yaml
uses
to sort map keys - that way we'll definitely be consistent with the
output of status, for example. See goyaml.keyList.Less for that
logic:
func less(a, b string) bool {
ar, br := []rune(a), []rune(b)
for i := 0; i < len(ar) && i < len(br); i++ {
if ar[i] == br[i] {
continue
}
al := unicode.
bl := unicode.
if al && bl {
return ar[i] < br[i]
}
if al || bl {
return bl
}
var ai, bi int
var an, bn int64
for ai = i; ai < len(ar) && unicode.
an = an*10 + int64(ar[ai]-'0')
}
for bi = i; bi < len(br) && unicode.
bn = bn*10 + int64(br[bi]-'0')
}
if an != bn {
return an < bn
}
if ai != bi {
return ai < bi
}
return ar[i] < br[i]
}
return len(ar) < len(br)
}
If we export the above function (probably from utils) then we'll have
a nice general routine that can be used for all kinds of ids
and tags and all the machine-id-specific parsing behaviour here
becomes unnecessary.
https:/
state/state.go:397: nrParts1 := len(idParts1)
From a performance point of view, there's no point in caching slice
length, and I actually suspect the code would read better with the more
direct len(idPartsX) rather than the extra temp vars.
https:/
File state/apiserver
https:/
state/apiserver
c.api.state.
I don't think this is quite right. state.EnsureAva
restructured some time ago so that it would be easy for that command to
return information on what it has done. Specifically, there's a struct
in there, "ensureAvailabi
information you'll need.
The only thing missing is the ids of the new state servers, but that
should be easily added.
Then you can easily produce output that reflects everything that
EnsureAvailability is doing, including making machines voting, removing
state server status, etc - all of which is very useful for a user to be
able to find out.
https:/