state/api: Introducing bulk operations on units

state/api: Introducing bulk operations on units

Proof-of-concept implementation of bulk server-side
API operations for units. The idea is not to change
the client-side interface, but replace the server-side
implementation to assume bulk operations by default.

Still WIP - for discussion.

https://code.launchpad.net/~dimitern/juju-core/050-api-bulk-ops-units/+merge/166195

(do not edit description out of merge proposal)

[dimitern, 2013-05-29 08:49:28]

Please take a look.

[fwereade, 2013-05-29 09:23:21]

Looks ideal in general. Just a few thoughts:

1) we need direct tests for the server over the API (we always did, really, it's
not a disadvantage of this approach)

2) I'm -1 on returning maps when we could return lists: there's at least one
case where we will need to specify ids as ints, and that'll be a problem in
json.

[jameinel, 2013-05-29 10:27:46]

https://codereview.appspot.com/9847044/diff/1/state/apiserver/units.go
File state/apiserver/units.go (right):

https://codereview.appspot.com/9847044/diff/1/state/apiserver/units.go#newcode23
state/apiserver/units.go:23: result.Units[name] = stateUnitToParams(unit)
I'm fine with William's comment that we probably want to default to a list of
objects rather than a mapping because of potential conflicts with name.
There are some simplicities of getting back data in exactly the format you use
it, but really we have to read the whole thing in the response. So reading a
list and shoving it into a map locally is really the same overhead as just
reading the map.

However, I notice here that if *any* unit name isn't found, it refuses to answer
all the rest of the request, and it only returns the simple error string. So you
are likely to not even know *which* unit failed.

Because of concurrency, I think we want to rethink the error logic.

Specifically, return:

[{"name": name, "unit": unitdetails, "error": null})

I'm thinking that otherwise if a unit happens to be killed by person X while
person Y is running 'juju status', status will just die, rather than returning
the best information it can get in a snapshot.

With a list of requested results, you have a way to say there was an error
retrieving information about unit "name".

And while there *is* some redundancy in the result, it makes it has a fair bit
of making context clear and error handling pretty straightforward.

[dimitern, 2013-05-31 06:55:47]

I'm closing this and will propose another branch with the agreed upon
implementation.