state/api: use error codes

state/api: use error codes

We add a general mechanism to rpc to transform
all server->client error codes, rather than add
this manually to each API entry point.

https://code.launchpad.net/~rogpeppe/juju-core/217-api-use-error-codes/+merge/149126

Requires: https://code.launchpad.net/~rogpeppe/juju-core/222-unexport-notfounderror/+merge/149104

(do not edit description out of merge proposal)

[rog, 2013-02-18 18:41:53]

Please take a look.

[dfc, 2013-02-18 22:42:36]

On 2013/02/18 18:41:53, rog wrote:
> Please take a look.

Please lbox propose again, diff is screwed.

[rog, 2013-02-18 23:02:32]

Please take a look.

[rog, 2013-02-18 23:03:21]

On 18 February 2013 22:42,  <dave@cheney.net> wrote:
> On 2013/02/18 18:41:53, rog wrote:
>>
>> Please take a look.
>
>
> Please lbox propose again, diff is screwed.

done. could have sworn i'd checked.

[dfc, 2013-02-18 23:15:20]

Yeah, that error doesn't seem to go away. 

On 19/02/2013, at 10:03, roger peppe <rogpeppe@gmail.com> wrote:

> On 18 February 2013 22:42,  <dave@cheney.net> wrote:
>> On 2013/02/18 18:41:53, rog wrote:
>>> 
>>> Please take a look.
>> 
>> 
>> Please lbox propose again, diff is screwed.
> 
> done. could have sworn i'd checked.

[dimitern, 2013-02-19 10:49:59]

LGTM, with some suggestions.

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine.go#newcode180
cmd/jujud/machine.go:180: log.Printf("jujud: api server has died: %v",
srv.Stop())
s/api/API ?

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:142: a := s.newAgent(c, m)
A short comment here will be nice, describing what's going on, even if it's
obvious for others (not so much for me though).

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go#newcode78
rpc/rpc_test.go:78: func (r *TRoot) ErrorMethods(id string) (*ErrorMethods,
error) {
It's a bit unclear for me why is this needed?

https://codereview.appspot.com/7314116/diff/9001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/7314116/diff/9001/rpc/server.go#newcode153
rpc/server.go:153: log.Printf("rpc: setting error code to %q", err.ErrorCode())
"rpc/server: ..." ?

https://codereview.appspot.com/7314116/diff/9001/state/api/error.go
File state/api/error.go (right):

https://codereview.appspot.com/7314116/diff/9001/state/api/error.go#newcode85
state/api/error.go:85: if err := err.(rpc.ErrorCoder); err != nil {
This feels a bit weird.. can you use theErr := .. or something instead?

[TheMue, 2013-02-19 16:54:55]

Mostly LGTM, but rpc_test.go would benefit from some helping commands.

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine.go#newcode180
cmd/jujud/machine.go:180: log.Printf("jujud: api server has died: %v",
srv.Stop())
On 2013/02/19 10:49:59, dimitern wrote:
> s/api/API ?

+1

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:147: defer a.Stop()
Why the defer not directly after the creation?

https://codereview.appspot.com/7314116/diff/9001/state/api/error.go
File state/api/error.go (right):

https://codereview.appspot.com/7314116/diff/9001/state/api/error.go#newcode85
state/api/error.go:85: if err := err.(rpc.ErrorCoder); err != nil {
On 2013/02/19 10:49:59, dimitern wrote:
> This feels a bit weird.. can you use theErr := .. or something instead?

To me it's ok. In case of a change better errc or cerr than theErr (see
clientError below).

[rog, 2013-02-20 09:35:40]

Please take a look.

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine.go#newcode180
cmd/jujud/machine.go:180: log.Printf("jujud: api server has died: %v",
srv.Stop())
On 2013/02/19 16:54:55, TheMue wrote:
> On 2013/02/19 10:49:59, dimitern wrote:
> > s/api/API ?
> 
> +1

Done.

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:142: a := s.newAgent(c, m)
On 2013/02/19 10:49:59, dimitern wrote:
> A short comment here will be nice, describing what's going on, even if it's
> obvious for others (not so much for me though). 

is the comment below not sufficient? all the tests call newAgent to create a new
agent, and newAgent itself has a comment. what do you find non-obvious here?

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:147: defer a.Stop()
On 2013/02/19 16:54:55, TheMue wrote:
> Why the defer not directly after the creation?

Done.

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go#newcode78
rpc/rpc_test.go:78: func (r *TRoot) ErrorMethods(id string) (*ErrorMethods,
error) {
On 2013/02/19 10:49:59, dimitern wrote:
> It's a bit unclear for me why is this needed?

i've divided the calls into three name spaces, based roughly on functionality.
each name space has its own accessor - this one returns an object with a method
that yields an error as required.

https://codereview.appspot.com/7314116/diff/9001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/7314116/diff/9001/rpc/server.go#newcode153
rpc/server.go:153: log.Printf("rpc: setting error code to %q", err.ErrorCode())
On 2013/02/19 10:49:59, dimitern wrote:
> "rpc/server: ..." ?

it was only for debugging - i forgot to remove it. thanks for spotting this.

for the record, we prefix log prints with the package path, not the file name.

https://codereview.appspot.com/7314116/diff/9001/state/api/error.go
File state/api/error.go (right):

https://codereview.appspot.com/7314116/diff/9001/state/api/error.go#newcode85
state/api/error.go:85: if err := err.(rpc.ErrorCoder); err != nil {
On 2013/02/19 10:49:59, dimitern wrote:
> This feels a bit weird.. can you use theErr := .. or something instead?

it's a common idiom, so will leave as is if that's ok; there's no need to invent
a new name when the old one will do just as well.

[dimitern, 2013-02-20 09:46:01]

Thanks for the answers!

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:142: a := s.newAgent(c, m)
On 2013/02/20 09:35:40, rog wrote:
> On 2013/02/19 10:49:59, dimitern wrote:
> > A short comment here will be nice, describing what's going on, even if it's
> > obvious for others (not so much for me though). 
> 
> is the comment below not sufficient? all the tests call newAgent to create a
new
> agent, and newAgent itself has a comment. what do you find non-obvious here?

I meant the goroutine and the defer

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go#newcode78
rpc/rpc_test.go:78: func (r *TRoot) ErrorMethods(id string) (*ErrorMethods,
error) {
On 2013/02/20 09:35:40, rog wrote:
> On 2013/02/19 10:49:59, dimitern wrote:
> > It's a bit unclear for me why is this needed?
> 
> i've divided the calls into three name spaces, based roughly on functionality.
> each name space has its own accessor - this one returns an object with a
method
> that yields an error as required.
I wasn't sure why a nil errorInst causes an error to be returned.

https://codereview.appspot.com/7314116/diff/9001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/7314116/diff/9001/rpc/server.go#newcode153
rpc/server.go:153: log.Printf("rpc: setting error code to %q", err.ErrorCode())
> for the record, we prefix log prints with the package path, not the file name.

Good to know, thanks.

[rog, 2013-02-20 10:48:58]

Please take a look.

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:142: a := s.newAgent(c, m)
On 2013/02/20 09:46:02, dimitern wrote:
> On 2013/02/20 09:35:40, rog wrote:
> > On 2013/02/19 10:49:59, dimitern wrote:
> > > A short comment here will be nice, describing what's going on, even if
it's
> > > obvious for others (not so much for me though). 
> > 
> > is the comment below not sufficient? all the tests call newAgent to create a
> new
> > agent, and newAgent itself has a comment. what do you find non-obvious here?
> 
> I meant the goroutine and the defer

i'll add a comment

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/7314116/diff/9001/rpc/rpc_test.go#newcode78
rpc/rpc_test.go:78: func (r *TRoot) ErrorMethods(id string) (*ErrorMethods,
error) {
On 2013/02/20 09:46:02, dimitern wrote:
> On 2013/02/20 09:35:40, rog wrote:
> > On 2013/02/19 10:49:59, dimitern wrote:
> > > It's a bit unclear for me why is this needed?
> > 
> > i've divided the calls into three name spaces, based roughly on
functionality.
> > each name space has its own accessor - this one returns an object with a
> method
> > that yields an error as required.
> I wasn't sure why a nil errorInst causes an error to be returned.

it's so that the transform-errors test can make sure that errors returned from
root-level accessor methods are correctly transformed.

[fwereade, 2013-02-20 13:54:19]

LGTM, just a suggestion I'm not really sure of myself.

https://codereview.appspot.com/7314116/diff/22001/state/api/client.go
File state/api/client.go (left):

https://codereview.appspot.com/7314116/diff/22001/state/api/client.go#oldcode105
state/api/client.go:105: Error     string
ErrorMessage, perhaps?

[rog, 2013-02-20 17:39:12]

*** Submitted:

state/api: use error codes

We add a general mechanism to rpc to transform
all server->client error codes, rather than add
this manually to each API entry point.

R=dfc, dimitern, TheMue, fwereade
CC=
https://codereview.appspot.com/7314116

https://codereview.appspot.com/7314116/diff/22001/state/api/client.go
File state/api/client.go (left):

https://codereview.appspot.com/7314116/diff/22001/state/api/client.go#oldcode105
state/api/client.go:105: Error     string
On 2013/02/20 13:54:19, fwereade wrote:
> ErrorMessage, perhaps?

hmm. i think i prefer just straight Error. i'll leave it like that unless you
object more strongly.