state: unit: AssignedMachine returns a machine

state: unit: AssignedMachine returns a machine

This proposal alters AssignedMachine (and it's signature) to return a 
*state.Machine rather than an id. All the non test consumers of this
method are actually after a *Machine, not the id anyway, so this saves them
the call to state.Machine(). The test code was altered for the new signature
but is operation is unaffected.

https://code.launchpad.net/~dave-cheney/juju-core/089-state-unit-assigned-machine/+merge/122617

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-09-04 06:32:11]

Please take a look.

[niemeyer, 2012-09-04 20:53:33]

The CL change doesn't seem to reflect the CL description.

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

https://codereview.appspot.com/6499071/diff/1/cmd/juju/status.go#newcode212
cmd/juju/status.go:212: if m, err := unit.AssignedMachine(); err == nil {
This seems to contradict the CL description. We used to have a machine id, and
now we will fetch a machine value from the database once more for every single
unit we have.

https://codereview.appspot.com/6499071/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6499071/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:113: if fw.machineds[machine.Id()] == nil {
Once again, real code that cares only about the id is now fetching the machine
just to pick its id.

[rog, 2012-09-06 12:04:41]

I'm +1 on this CL. It's easy to get a machine id from a Machine, but getting a
Machine from an id requires an additional round trip.

[niemeyer, 2012-09-06 12:55:09]

On 2012/09/06 12:04:41, rog wrote:
> I'm +1 on this CL. It's easy to get a machine id from a Machine, but getting a
> Machine from an id requires an additional round trip.

Thus we should *always* be paying for the additional roundtrip to the database,
even when the two only non-test locations involved don't actually want anything
but the machine id? I don't understand.

This seems like a totally straightforward decision, btw. It concerns me a bit
that we're even discussing it. I'll soon be stepping down as core reviewer and
will not be overseeing all such changes anymore.