Merge lp://staging/~dimitern/juju-core/050-api-bulk-ops-units-2 into lp://staging/~juju/juju-core/trunk

Reviewers: mp+166766_code.launchpad.net,

Message:
Please take a look.

Description:
state/api: Bulk unit operations; internal tests

This introduces srvUnits as an entry point for
bulk API operations on units, as agreed.
srvUnit is removed and the client-side API was
refactored to use the bulk operations instead.

Also added much needed internal API server tests.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9797046/

Affected files:
   A [revision details]
   M state/api/params/params.go
   M state/api/unit.go
   M state/apiserver/api_test.go
   M state/apiserver/apierror.go
   M state/apiserver/apiserver.go
   M state/apiserver/export_test.go
   A state/apiserver/internal_test.go
   M state/apiserver/login_test.go
   M state/apiserver/perm_test.go
   M state/apiserver/root.go
   D state/apiserver/unit.go
   M state/apiserver/unit_test.go
   A state/apiserver/units.go
   M state/apiserver/utils.go

Please take a look.

https://codereview.appspot.com/9797046/

Please take a look.

https://codereview.appspot.com/9797046/

Please take a look.

https://codereview.appspot.com/9797046/

Please take a look.

https://codereview.appspot.com/9797046/

Thanks for working on this. Especially thanks for being the guinea pig
and pushing forward. I think the first couple of APIs we'll work out
some common practices and the others should go faster.

I'd like to get your feedback about structs-of-arrays vs
arrays-of-structs (see inline), and maybe some other people's opinions
as well. It is certainly just a gut feeling on my part, but I really
like locality of params.

https://codereview.appspot.com/9797046/diff/8001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/9797046/diff/8001/state/api/params/params.go#newcode220
state/api/params/params.go:220: Passwords []Password
While I understand this is easier to write (two slices of basic types)
it seems as an API I would rather group it as:

type NamePassword struct {
  Name string
  Password Password
}

type UnitsSetPassword struct {
   Passwords []NamePassword
}

I'm not sure the exact syntax because I don't really like variables
exactly matching types, but someone decided to make the type Password,
so I'm not sure if we can avoid it.
Anyway, to give my motivation, imagine two api calls:

SetPasswords(["a", "b", "q", "r", "n", "z", "t", "l"],
  ["xxxyyy", "abcdoeu", "stduf", "aaqueu", "ztoheu", "haoboehu",
"thoeu"])

Do you know without counting what the password for "r" is? Do you know
if the number of Names matches the number of Passwords? Compare that
with either:

SetPasswords([["a", "xxxyyy"], ["b", "abcdoeu"], ["q", "stduf"],
               ["r", "aaqueu"], ["n", "ztoheu"], ["z", "haoboehu"],
               ["t", "thoeu"], ["l", ""], ],)

I realize that in JSON serialization this probably adds bytes of
overhead on the wire to:

SetPasswords([{"Name": "a", "Password": "xxxyyy"},
     {"Name": "b", "Password": "abcdoeu"},
     {"Name": "q", "Password": "stduf"],
     {"Name": "r", "Password": "aaqueu"},
     {"Name": "n", "Password": "ztoheu"},
     {"Name": "z", "Password": "haoboehu"},
     {"Name": "t", "Password": "thoeu"},
     {"Name": "l", "Password": ""},
])

That is quite a bit of overhead (more than the actual content). However,
it has great locality, and if you got that in a debug log you could
immediately figure out what the request was, what each parameter meant,
and what it applied to.

In general, I think it makes more sense for APIs to take
arrays-of-structs rather than structs-of-arrays.

I'm willing to be persuaded otherwise, but it is certainly my default
stance.

https://codereview.appspot.com/9797046/diff/8001/state/api/params/params.go#newcode249
state/api/params/params.go:249: }
Here it is fine, because the "struct" or "parameter" for each thing you
are getting is just the simple Name string.

https://codereview.appspot.com/9797046/diff/8001/state/api/unit.go
File state/api/unit.go (right):

https://codereview.appspot.com/9797046/diff/8001/state/api/unit.go#newcode65
state/api/unit.go:65: Passwords: []params.Password{{password}},
For one unit, the syntax is roughly the same. For many units the syntax
would be a bit clumsier, except you are going to do it programmatically
in a for loop anyway. And there you would have:

names = make([]string, len(units))
passwords = mak...

Thanks for the review!

I'm proposing shortly an updated version with most of your excellent
suggestions implemented.

https://codereview.appspot.com/9797046/diff/8001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/9797046/diff/8001/state/api/params/params.go#newcode220
state/api/params/params.go:220: Passwords []Password
On 2013/05/31 13:05:12, jameinel wrote:
> While I understand this is easier to write (two slices of basic types)
it seems
> as an API I would rather group it as:

> type NamePassword struct {
> Name string
> Password Password
> }

> type UnitsSetPassword struct {
> Passwords []NamePassword
> }

> I'm not sure the exact syntax because I don't really like variables
exactly
> matching types, but someone decided to make the type Password, so I'm
not sure
> if we can avoid it.
> Anyway, to give my motivation, imagine two api calls:

> SetPasswords(["a", "b", "q", "r", "n", "z", "t", "l"],
> ["xxxyyy", "abcdoeu", "stduf", "aaqueu", "ztoheu", "haoboehu",
"thoeu"])

> Do you know without counting what the password for "r" is? Do you know
if the
> number of Names matches the number of Passwords? Compare that with
either:

> SetPasswords([["a", "xxxyyy"], ["b", "abcdoeu"], ["q", "stduf"],
> ["r", "aaqueu"], ["n", "ztoheu"], ["z", "haoboehu"],
> ["t", "thoeu"], ["l", ""], ],)

> I realize that in JSON serialization this probably adds bytes of
overhead on the
> wire to:

> SetPasswords([{"Name": "a", "Password": "xxxyyy"},
> {"Name": "b", "Password": "abcdoeu"},
> {"Name": "q", "Password": "stduf"],
> {"Name": "r", "Password": "aaqueu"},
> {"Name": "n", "Password": "ztoheu"},
> {"Name": "z", "Password": "haoboehu"},
> {"Name": "t", "Password": "thoeu"},
> {"Name": "l", "Password": ""},
> ])

> That is quite a bit of overhead (more than the actual content).
However, it has
> great locality, and if you got that in a debug log you could
immediately figure
> out what the request was, what each parameter meant, and what it
applied to.

> In general, I think it makes more sense for APIs to take
arrays-of-structs
> rather than structs-of-arrays.

> I'm willing to be persuaded otherwise, but it is certainly my default
stance.

Very good point, thanks! I'll update the proposal to use
arrays-of-structs. I wasn't really thinking I confess :) But definitely
it's better for debugging and matching what goes with what. The expense
of extra bytes on the wire shouldn't be a concern. If it is, we can deal
with it at a later stage.

https://codereview.appspot.com/9797046/diff/8001/state/api/params/params.go#newcode249
state/api/params/params.go:249: }
On 2013/05/31 13:05:12, jameinel wrote:
> Here it is fine, because the "struct" or "parameter" for each thing
you are
> getting is just the simple Name string.

Yep.

https://codereview.appspot.com/9797046/diff/8001/state/api/unit.go
File state/api/unit.go (right):

https://codereview.appspot.com/9797046/diff/8001/state/api/unit.go#newcode65
state/api/unit.go:65: Passwords: []params.Password{{password}},
On 2013/05/31 13:05:12, jameinel wrote:
> For one unit, the syntax is roughly t...

Please take a look.

https://codereview.appspot.com/9797046/

Sorry for the spam (yet again), but I forgot to change Units.Get as
suggested by John (arrays-of-structs). Also removed the TODOs for bulk
op methods, because wasn't sure what to put there.

https://codereview.appspot.com/9797046/

Please take a look.

https://codereview.appspot.com/9797046/

See my top level comments here are similar to those made for the other
branch. Remote calls should all be routed via the service layer and not
attached to domain objects. I think it would be easier to discuss this
on a call rather than back and forth via email.

https://codereview.appspot.com/9797046/

On 2013/06/03 05:42:20, wallyworld wrote:
> See my top level comments here are similar to those made for the other
branch.
> Remote calls should all be routed via the service layer and not
attached to
> domain objects. I think it would be easier to discuss this on a call
rather than
> back and forth via email.

Excerpt from IRC logs:
[08:18:51] <dimitern> wallyworld: client-side api stays as in state, as
agreed on the last call - we cannot change that now
[08:19:07] <dimitern> wallyworld: it'll require rewritting everything
[08:19:31] <dimitern> wallyworld: but it's a transitional state towards
having bulk support on the server
[08:19:56] <dimitern> wallyworld: and eventually the agents can benefit
from that (one at a time) when refactored
[08:22:25] <wallyworld> dimitern: we need to discuss. i'm on a call
right now. perhaps after standup. the way it is now is fundamentally
broken because it is 1. stateful, 2. not connectionless, 3. not service
oriented
[08:23:11] <dimitern> wallyworld: none of these 3 things can change in a
single CL
<dimitern> wallyworld: as i said, changing the client-side api needs to
happen once the state api itself is changed accordingly
<dimitern> wallyworld: what's important is to have the api server-side
right first, which is what the cl does
<dimitern> wallyworld: but by all means, another discussion is in order
with me, you, fwereade and rogpeppe

https://codereview.appspot.com/9797046/

On 2013/06/03 07:39:11, dimitern wrote:
> On 2013/06/03 05:42:20, wallyworld wrote:
> > See my top level comments here are similar to those made for the
other branch.
> > Remote calls should all be routed via the service layer and not
attached to
> > domain objects. I think it would be easier to discuss this on a call
rather
> than
> > back and forth via email.

> Excerpt from IRC logs:
> [08:18:51] <dimitern> wallyworld: client-side api stays as in state,
as agreed
> on the last call - we cannot change that now
> [08:19:07] <dimitern> wallyworld: it'll require rewritting everything
> [08:19:31] <dimitern> wallyworld: but it's a transitional state
towards having
> bulk support on the server
> [08:19:56] <dimitern> wallyworld: and eventually the agents can
benefit from
> that (one at a time) when refactored
> [08:22:25] <wallyworld> dimitern: we need to discuss. i'm on a call
right now.
> perhaps after standup. the way it is now is fundamentally broken
because it is
> 1. stateful, 2. not connectionless, 3. not service oriented
> [08:23:11] <dimitern> wallyworld: none of these 3 things can change in
a single
> CL
> <dimitern> wallyworld: as i said, changing the client-side api needs
to happen
> once the state api itself is changed accordingly
> <dimitern> wallyworld: what's important is to have the api server-side
right
> first, which is what the cl does
> <dimitern> wallyworld: but by all means, another discussion is in
order with me,
> you, fwereade and rogpeppe

As agreed today, I'm closing this and moving towards worker/facade based
implementation for Machiner first, keeping the bulk operations on the
methods on the wire, but keeping domain-level objects at client-side,
proxying through the facade.

https://codereview.appspot.com/9797046/