Implement a SetAnnotations API call.

Implement a SetAnnotations API call.

SetAnnotations now sets multiple annotations
on an entity. This is required by the GUI, in
order to have one rpc call for an atomic operation
involving several annotations.

https://code.launchpad.net/~frankban/juju-core/bug-1152652-set-annotations/+merge/152448

Requires: https://code.launchpad.net/~frankban/juju-core/ann-doc/+merge/152280

(do not edit description out of merge proposal)

[frankban, 2013-03-08 16:32:04]

Please take a look.

[matthew.scott, 2013-03-08 16:34:12]

This merge includes the previous API branch in the diff against trunk.  The
methods that were changed were in state/api/apiclient.go,
state/api/params/params.go, and state/apiserver/apiserver.go, along with the
associated tests.

[frankban, 2013-03-08 18:15:57]

Please take a look.

[dimitern, 2013-03-08 18:58:44]

LGTM

[dave_cheney.net, 2013-03-08 20:09:18]

On 2013/03/08 18:58:44, dimitern wrote:
> LGTM

NOT LGTM. This diff is dirty. Please pull trunk and repropose.

[fwereade, 2013-03-09 16:44:32]

On 2013/03/08 20:09:18, dfc wrote:
> On 2013/03/08 18:58:44, dimitern wrote:
> > LGTM
> 
> NOT LGTM. This diff is dirty. Please pull trunk and repropose.

+1, I'm afraid I can't read this.

[fwereade, 2013-03-09 16:54:36]

just a comment, will follow up properly when the diff's clean

https://codereview.appspot.com/7644043/diff/4002/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7644043/diff/4002/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:364: err := entity.SetAnnotation(key, value)
Can someone comment on the actual riskiness of this behaviour? Looks racy to me,
given that multiple clients will want to set multiple annotations at a time; but
maybe it's ok, if the annotations won't actually overlap.

[frankban, 2013-03-13 13:21:38]

We will re-propose a clean branch after landing the prerequisite one.
As a note: William suggested that we switch to setAnnotations (trailing "s")
also in the internal layer.

[fwereade, 2013-03-15 14:54:23]

On 2013/03/13 13:21:38, frankban wrote:
> We will re-propose a clean branch after landing the prerequisite one.
> As a note: William suggested that we switch to setAnnotations (trailing "s")
> also in the internal layer.

I'm setting WIP, the propose will move it back into the queue when it's ready.

[frankban, 2013-03-20 16:53:17]

Please take a look.

[frankban, 2013-03-20 16:56:30]

Here is the clean diff for this branch.
Switched to setAnnotations also in the mongo layer.
As already mentioned, this branch still does not solve the potential race
conditions in *annotator.SetAnnotations. 
Having this branch merged, the GUI will be able to use the annotations
machinery. The race issue is the goal of another branch.

[fwereade, 2013-03-20 17:58:13]

This LGTM for the stated purpose. Hopefully some of the content of
https://codereview.appspot.com/7631046/ will assist you in landing the followup
without too much pain.

https://codereview.appspot.com/7644043/diff/20001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7644043/diff/20001/state/annotator.go#newcode89
state/annotator.go:89: if err := a.st.runner.Run(ops, "", nil); err != nil {
This is fine for now, but it would be great if you'd assign a bug to yourself
and note the number in a TODO here.

FWIW https://codereview.appspot.com/7631046/ has a rudimentary hacking doc that
may help with context for what I was asking for here. I'd be very keen to hear
what it's missing.

[frankban, 2013-03-21 09:37:13]

Please take a look.

https://codereview.appspot.com/7644043/diff/20001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7644043/diff/20001/state/annotator.go#newcode89
state/annotator.go:89: if err := a.st.runner.Run(ops, "", nil); err != nil {
On 2013/03/20 17:58:14, fwereade wrote:
> This is fine for now, but it would be great if you'd assign a bug to yourself
> and note the number in a TODO here.
> 
> FWIW https://codereview.appspot.com/7631046/ has a rudimentary hacking doc
that
> may help with context for what I was asking for here. I'd be very keen to hear
> what it's missing.

Done.

[rog, 2013-03-21 10:44:11]

LGTM, very nice (on the understanding that the transaction retry logic is
implemented in a later branch)

https://codereview.appspot.com/7644043/diff/27001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7644043/diff/27001/state/annotator.go#newcode29
state/annotator.go:29: func (a *annotator) SetAnnotations(pairs
map[string]string) error {
nice clear logic in this function.

[frankban, 2013-03-21 10:48:58]

*** Submitted:

Implement a SetAnnotations API call.

SetAnnotations now sets multiple annotations
on an entity. This is required by the GUI, in
order to have one rpc call for an atomic operation
involving several annotations.

R=matthew.scott, dimitern, dfc, fwereade, rog
CC=
https://codereview.appspot.com/7644043