state: use string IDs for machines

state: use string IDs for machines

ok, this is *huge*, but most changes are just s/int/string/ and those left
over are (1) obvious and (2) for the better IMO.

https://code.launchpad.net/~fwereade/juju-core/machine-string-ids/+merge/135423

(do not edit description out of merge proposal)

[fwereade, 2012-11-21 14:40:13]

Please take a look.

[TheMue, 2012-11-21 15:53:54]

LGTM, only have comments that we typically quote string ids with %q. But later
I've seen that valid ids are only [0-9]+, so %s should be ok (same error message
text as before).

https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go#newcode70
cmd/juju/ssh.go:70: log.Printf("cmd/juju: looking up address for machine %s...",
target)
%q instead of %s

https://codereview.appspot.com/6852080/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6852080/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:665: panic(fmt.Errorf("ClosePorts with mismatched
machine id, expected %s got %s", inst.machineId, machineId))
%q instead of %s

https://codereview.appspot.com/6852080/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6852080/diff/1/environs/ec2/ec2.go#newcode367
environs/ec2/ec2.go:367: log.Printf("environs/ec2: starting machine %s in %q
running tools version %q from %q", scfg.machineId, e.name, scfg.tools.Binary,
scfg.tools.URL)
%q instead of %s

https://codereview.appspot.com/6852080/diff/1/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/6852080/diff/1/state/assign_test.go#newcode518
state/assign_test.go:518: c.Errorf("machine %s expected one unit, got %q", id,
us)
%q instead of %s

[fwereade, 2012-11-21 17:09:55]

On 2012/11/21 15:53:54, TheMue wrote:
> LGTM, only have comments that we typically quote string ids with %q. But later
> I've seen that valid ids are only [0-9]+, so %s should be ok (same error
message
> text as before).
> 
> https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go
> File cmd/juju/ssh.go (right):
> 
> https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go#newcode70
> cmd/juju/ssh.go:70: log.Printf("cmd/juju: looking up address for machine
%s...",
> target)
> %q instead of %s
> 
> https://codereview.appspot.com/6852080/diff/1/environs/dummy/environs.go
> File environs/dummy/environs.go (right):
> 
>
https://codereview.appspot.com/6852080/diff/1/environs/dummy/environs.go#newc...
> environs/dummy/environs.go:665: panic(fmt.Errorf("ClosePorts with mismatched
> machine id, expected %s got %s", inst.machineId, machineId))
> %q instead of %s
> 
> https://codereview.appspot.com/6852080/diff/1/environs/ec2/ec2.go
> File environs/ec2/ec2.go (right):
> 
> https://codereview.appspot.com/6852080/diff/1/environs/ec2/ec2.go#newcode367
> environs/ec2/ec2.go:367: log.Printf("environs/ec2: starting machine %s in %q
> running tools version %q from %q", scfg.machineId, e.name, scfg.tools.Binary,
> scfg.tools.URL)
> %q instead of %s
> 
> https://codereview.appspot.com/6852080/diff/1/state/assign_test.go
> File state/assign_test.go (right):
> 
> https://codereview.appspot.com/6852080/diff/1/state/assign_test.go#newcode518
> state/assign_test.go:518: c.Errorf("machine %s expected one unit, got %q", id,
> us)
> %q instead of %s

Yeah, I felt that avoiding changes where possible was sensible.

[niemeyer, 2012-11-21 23:08:07]

Representing ints as strings honestly feels like a lesser design, and this
specific branch doesn't feel like a positive change. I really hope the this
change justifies itself due to what's coming.

LGTM, assuming you're willing to revert it if it proves unjustified by further
changes.

https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go#newcode70
cmd/juju/ssh.go:70: log.Printf("cmd/juju: looking up address for machine %s...",
target)
On 2012/11/21 15:53:54, TheMue wrote:
> %q instead of %s

%s is right in this case. We want `machine 1`, not `machine "1"`

https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go#newcode83
cmd/juju/ssh.go:83: return "", fmt.Errorf("unknown unit or machine %q", target)
%s

https://codereview.appspot.com/6852080/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6852080/diff/1/cmd/jujud/machine.go#newcode40
cmd/jujud/machine.go:40: return fmt.Errorf("--machine-id option must be set, and
expects a non-negative integer")
"... must be set to a positive integer"?

https://codereview.appspot.com/6852080/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6852080/diff/1/state/state.go#newcode239
state/state.go:239: sort.Sort(mdocs)
Somewhat sad that we have to do this. AllMachines is broken by nature, since we
can't really pull all machines out in a world of 100k machines. I guess we've
lost the ability to return them in a sorted fashion for iteration for good?

https://codereview.appspot.com/6852080/diff/1/state/state.go#newcode246
state/state.go:246: type machineSlice []machineDoc
machineDocSlice, or machineDocs (this is not a slice of machines).

https://codereview.appspot.com/6852080/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6852080/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:53: machines:  make(map[string]string),
I could infer what the old style meant due to the types, but the new one needs
explanations now: string => string? (string => instance, except the string isn't
the instance id?) 

The logic in this file will be especially prone to value confusion.

[fwereade, 2012-11-22 08:41:21]

OK, I'm somewhat nervous about the "ok if you're willing to revert" sentiment.
This is partly because I don't really understand what critera you'd be using to
expect such a thing, and partly because... well, I kinda assume that there's
always a possibility that a change will, at some stage, turn out to be a bad
idea and need to be reverted, and the request for explicit agreement to such an
approach makes me think you're *expecting* that to happen.

Would you expand a little on what's going through your mind here?

https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/6852080/diff/1/cmd/juju/ssh.go#newcode83
cmd/juju/ssh.go:83: return "", fmt.Errorf("unknown unit or machine %q", target)
On 2012/11/21 23:08:08, niemeyer wrote:
> %s

I don't think so, in this instance -- there's tension between quoting a unit
name and a machine id, but I think both considerations are overridden by the
fact that we're complaining that we can't understand a string typed by the user:
quoting the thing we're complaining about seems appropriate in this case.

https://codereview.appspot.com/6852080/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6852080/diff/1/cmd/jujud/machine.go#newcode40
cmd/jujud/machine.go:40: return fmt.Errorf("--machine-id option must be set, and
expects a non-negative integer")
On 2012/11/21 23:08:08, niemeyer wrote:
> "... must be set to a positive integer"?

I think that way way back we agreed on "non-negative" on the basis that 0 is
valid, and neither positive or negative.

https://codereview.appspot.com/6852080/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6852080/diff/1/state/state.go#newcode239
state/state.go:239: sort.Sort(mdocs)
On 2012/11/21 23:08:08, niemeyer wrote:
> Somewhat sad that we have to do this. AllMachines is broken by nature, since
we
> can't really pull all machines out in a world of 100k machines. I guess we've
> lost the ability to return them in a sorted fashion for iteration for good?

I think so, yes. In general I question the idea that the ordering of machines is
actually meaningful; my gut says we shouldn't be worrying about producing sorted
output from state at all, because it forces us to do additional work based on
assumptions about our clients that may not actually hold. Mileage may ofc vary
:).

https://codereview.appspot.com/6852080/diff/1/state/state.go#newcode246
state/state.go:246: type machineSlice []machineDoc
On 2012/11/21 23:08:08, niemeyer wrote:
> machineDocSlice, or machineDocs (this is not a slice of machines).

Good point, thank you.

https://codereview.appspot.com/6852080/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6852080/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:53: machines:  make(map[string]string),
On 2012/11/21 23:08:08, niemeyer wrote:
> I could infer what the old style meant due to the types, but the new one needs
> explanations now: string => string? (string => instance, except the string
isn't
> the instance id?) 
> 
> The logic in this file will be especially prone to value confusion.

I personally subscribe to a slightly more hardline "understand the concepts in
play before you mess with the code" view, but your point is well taken. Followup
branch with `type InstanceId string`?

[fwereade, 2012-11-22 08:45:31]

On 2012/11/21 23:08:07, niemeyer wrote:
> Representing ints as strings honestly feels like a lesser design, and this
> specific branch doesn't feel like a positive change. I really hope the this
> change justifies itself due to what's coming.

I'm not sure that there's anything about a machine ID that strongly indicates it
"should" be an int. Mathematical operations are meaningless; sorting STM to be
meaningless as well; and the received wisdom seems to indicate that the
theoretical superiority of indexed ints over indexed strings will only really
come into play when the strings are "long" -- say a few hundred chars.

What am I missing or misunderstanding?

[fwereade, 2012-11-22 09:31:48]

On 2012/11/22 08:45:31, fwereade wrote:
> On 2012/11/21 23:08:07, niemeyer wrote:
> > Representing ints as strings honestly feels like a lesser design, and this
> > specific branch doesn't feel like a positive change. I really hope the this
> > change justifies itself due to what's coming.

Another minor but IMO real benefit: a zero string is not a valid machine, but a
zero int is, and this is a tedious and confusing hassle (which we handle in
different ways in different places (-1, pointer) for both relations and
machines).

[niemeyer, 2012-11-22 13:38:14]

On Thu, Nov 22, 2012 at 6:45 AM,  <fwereade@gmail.com> wrote:
> I'm not sure that there's anything about a machine ID that strongly
> indicates it "should" be an int. Mathematical operations are
> meaningless; sorting STM to be meaningless as well; and the received
> wisdom seems to indicate that the theoretical superiority of indexed
> ints over indexed strings will only really come into play when the
> strings are "long" -- say a few hundred chars.
>
> What am I missing or misunderstanding?

I think the disagreement is that it *is* an integer, no matter whether
it should be or not. I also don't understand your argument about
indexing. Among other things, the current change is dropping the use
of indexes because it simply can't use them for sorting. Also, even
disconsidering that, storing "100" in a BSON string takes 4+3+1 bytes,
vs. 4 for ints. Not to mention that []int is a block of memory, while
[]string is a block of pointers to individual memory slots.

I'm not going to block this, but *this* CL is clearly a bad trade-off.
I'm really hoping that it's justified by changes that are more
relevant than the disadvantages it's bringing.

[fwereade, 2012-11-28 15:35:31]

*** Submitted:

state: use string IDs for machines

ok, this is *huge*, but most changes are just s/int/string/ and those left
over are (1) obvious and (2) for the better IMO.

R=
CC=
https://codereview.appspot.com/6852080

[aram, 2012-12-03 16:27:34]

Finally we have this! Thank you William.