Code review comment for lp:~axwalk/juju-core/lp1121916-juju-status-filters

Revision history for this message
William Reade (fwereade) wrote :

Thanks, this is a great start, but we need to think about subordinates
in a bit more detail. Thoughts below.

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

https://codereview.appspot.com/12235043/diff/7001/cmd/juju/status.go#newcode113
cmd/juju/status.go:113: mid, err := unit.AssignedMachineId()
There's something here that demands a bit more thought.

* principal units have to(?) be included if the filter matches one of
their subordinates (because they're displayed in the principal's
"subordinates" field)
* subordinate units should be included if the filter matches their
principal (because otherwise we're stripping relevant info from the
requested unit)
* if a principal is included only because one of its subordinates is, we
probably shouldn't display its other subordinates, because they'renot
relevant to the user's request
* AssignedMachineId is expensive for subordinate units, because it
roundtrips to get the machine id from the principal

I *think* that if we tweak unit filtering as above (which remains open
to argument, but is I think consistent with the analogous behaviour for
machines) we can just skip all subordinate units here and still be
correct, and not waste any further time chatting to the database.

https://codereview.appspot.com/12235043/

« Back to merge proposal