rpc: support a Version field

rpc: support a Version field

This adds support for a Version field in RPC requests, and changes the
MethodCaller interface for how and when it looks up objects.
Most of the MethodCaller changes were already reviewed in:
 https://code.launchpad.net/~jameinel/juju-core/reflect-only-facades/+merge/218764
There are only a couple of tweaks because the lookup is now done with a
real Version instead of overriding the "id" field.

This also uses the registry.TypedNameVersion code in a sample registry that
shows one possible way that we can do a Registry with versioned lookups.
Note that the next steps don't actually make use of rpc/registry/* so
I'm willing to pull it out, or put it in as a test-suite only thing.
Alternatively it might be something tha could fit into
documentation/example sort of code.

This does disable one test that assumed you could statically iterate
over all the types and exposed methods. I introduce a similar function
later on once we have the factories registered.

One thing that was brought up before about always unfolding Interface(),
I didn't do that yet, but I still could if we feel it would be more
straightforward.

https://code.launchpad.net/~jameinel/juju-core/api-rpc-reflect-version/+merge/219653

Requires: https://code.launchpad.net/~jameinel/juju-core/typed-registry/+merge/219161

(do not edit description out of merge proposal)

[jameinel, 2014-05-15 08:18:54]

Please take a look.

[fwereade, 2014-05-16 09:05:28]

LGTM

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go
File rpc/reflect_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go#newcode58
rpc/reflect_test.go:58: // Is this worth specifying?
I still think it is :).

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newco...
rpc/registry/registry.go:30: // could just pass Factory instead of this pointer
Elem trick
Tell me about it.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newco...
rpc/registry/registry.go:50: ) {
/me feels quietly smug

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newco...
rpc/registry/registry.go:62: // There was a problem, do we translate it into
CallNotImplementedError ?
I *think* this is good as it is.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go
File rpc/registry/registry_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go#...
rpc/registry/registry_test.go:47: // c.Check(err, gc.ErrorMatches, `unknown
object type "facade"`)
d

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go#newcode789
rpc/rpc_test.go:789: // and beyond
I'm really appreciating the comments, but I'd love to see trailing full stops.

[jameinel, 2014-05-16 10:43:39]

Please take a look.

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go
File rpc/reflect_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go#newcode58
rpc/reflect_test.go:58: // Is this worth specifying?
On 2014/05/16 09:05:27, fwereade wrote:
> I still think it is :).

I wasn't sure because the test used to assert that rtype.ObjType matched
rpcreflect.ObjTypeOf() (so that the object type being exposed to the world
matched the object type you get by looking it up directly.)
Since we just asserted that the actual reflect.Type is identical, it seemed a
little strange to assert that ObjTypeOf(T), gc.Equals, ObjTypeOf(T).

But I'm willing to leave it in.

Comment removed.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newco...
rpc/registry/registry.go:50: ) {
On 2014/05/16 09:05:28, fwereade wrote:
> /me feels quietly smug

Having seen quite a bit of code with long params jumping around we do have quite
a few places that also do it Nate's way (possibly written by him):

func (r *RootFromRegistry) MethodCaller(
	rootMethodName string, version int, objId, objMethodName string,
) (rcreflect.MethodCaller, error) {

I think I still prefer the consistency between args in and args out.

The only case I might switch is when the arg list is still too long because of
package names, etc. I haven't quite decided between:

func (r *RootFromRegistry) MethodCaller(
	rootMethodName string, version int,
        objId string, objMethodName string,
) (
	rpcreflect.MethodCaller, error,
) {

and

func (r *RootFromRegistry) MethodCaller(
	rootMethodName string,
        version int,
        objId string,
        objMethodName string,
) (
	rpcreflect.MethodCaller, error,
) {

(If you *have* to wrap, should you go down to one-per-line).

Anyway, I did like your suggestion, and certainly I asked looking for a nice way
to go. :)

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newco...
rpc/registry/registry.go:62: // There was a problem, do we translate it into
CallNotImplementedError ?
On 2014/05/16 09:05:28, fwereade wrote:
> I *think* this is good as it is.

comment removed. I could see a case for mapping obvious types, like potentially
a NotFound into a CallNotImplemented.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go
File rpc/registry/registry_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go#...
rpc/registry/registry_test.go:47: // c.Check(err, gc.ErrorMatches, `unknown
object type "facade"`)
On 2014/05/16 09:05:28, fwereade wrote:
> d

Done.

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go#newcode789
rpc/rpc_test.go:789: // and beyond
On 2014/05/16 09:05:28, fwereade wrote:
> I'm really appreciating the comments, but I'd love to see trailing full stops.

I went through and audited the change so far, adding full stops where
appropriate (hopefully).

[dimitern, 2014-05-19 09:33:06]

Few comments for clarification, otherwise looks great!

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#n...
rpc/registry/registry.go:20: func (r *RootFromRegistry) Register(name string,
version int, f Factory) {
Doc comment? Also, is panicking here a good idea?

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#n...
rpc/registry/registry.go:45: //	objId: facadeVersion (will be translated from a
string into an int)
This comment is a bit confusing - we have version int and objId string - is only
the latter used for versioning?

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go#newcode344
rpc/server.go:344: // version, id, methodName) triplet.
It's a quadruplet now actually :)

[jameinel, 2014-05-22 11:42:14]

Please take a look.

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#n...
rpc/registry/registry.go:20: func (r *RootFromRegistry) Register(name string,
version int, f Factory) {
On 2014/05/19 09:33:06, dimitern wrote:
> Doc comment? Also, is panicking here a good idea? 

So this is example code, but panic() in stuff that happens during init() is fine
with me. Because then it panics early and not just end up with things that
aren't actually registered.

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#n...
rpc/registry/registry.go:45: //	objId: facadeVersion (will be translated from a
string into an int)
On 2014/05/19 09:33:06, dimitern wrote:
> This comment is a bit confusing - we have version int and objId string - is
only
> the latter used for versioning?

Just out of date docstrings. It was added when we were using objId for
versioning, but now we have the actual Version field. I updated the comments.

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go#newcode344
rpc/server.go:344: // version, id, methodName) triplet.
On 2014/05/19 09:33:06, dimitern wrote:
> It's a quadruplet now actually :)

Done.