New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ClusterRpc DependencyProvider #585

Merged
merged 7 commits into from Nov 19, 2018

Conversation

Projects
None yet
2 participants
@mattbennett
Copy link
Contributor

mattbennett commented Oct 29, 2018

As discussed on the forum.

A dynamic RPC proxy DependencyProvider has been requested several times before, and the refactor landing in the v3.x release makes it much easier to provide.

This change simply makes the target_service optional when instantiating the RpcProxy.

@davidszotten

This comment has been minimized.

Copy link
Member

davidszotten commented Oct 30, 2018

though this is an easy way to implement it, i wonder if this makes for a confusing api, especially for new users who may not understand the somewhat subtle difference

maybe we should have two different (eg thin wrappers) for a "cluster" vs "service" proxy

@mattbennett

This comment has been minimized.

Copy link
Contributor

mattbennett commented Oct 30, 2018

That's a good point. Introducing this as a new feature would make it easier to communicate too.

We use the thin wrapper pattern in the standalone package. In the v3 branch I renamed the standalone ClusterRpcProxy to simply RpcProxy, which is unfortunate if we want to alias the ServiceRpcProxy DP to RpcProxy for backwards compatibility here.

I think the best thing to do is roll back that change so we end up with a ServiceRpcProxy and ClusterRpcProxy in both the standalone and normal packages. We should add an RpcProxy alias to ServiceRpcProxy in the normal package to maintain backwards compatibility, and perhaps switch the new standalone RpcProxy alias to point to the ServiceRpcProxy instead, so it matches.

@davidszotten

This comment has been minimized.

Copy link
Member

davidszotten commented Oct 30, 2018

sounds reasonable.

though this made me think of something else: if we are considering changing the names, maybe we should also put the word Proxy on the table. not sure how we landed on that name in the first place, but maybe e.g Client or something else would make a better name

@mattbennett

This comment has been minimized.

Copy link
Contributor

mattbennett commented Oct 30, 2018

Ha! Yes, Proxy has always been awkward. What do you think about the following?

  • RpcProxy / ServiceRpcProxy => ServiceRpc or ServiceRpcClient or ServiceClient
  • ClusterRpcProxy => ClusterRpc or ClusterRpcClient or ClusterClient

Or possibly one object called RpcClient with the API as currently implemented, service name optional. Maybe less confusing if we're replacing the existing API with a brand new one?

I think my preference is ServiceRpc and ClusterRpc.

@davidszotten

This comment has been minimized.

Copy link
Member

davidszotten commented Oct 30, 2018

I don't have any strong views on actual names, though i think i prefer two separate ones. ServiceRpc and ClusterRpc seem fine

@mattbennett mattbennett changed the title target_service now optional on RpcProxy declaration Add ClusterRpc DependencyProvider Oct 31, 2018

@mattbennett

This comment has been minimized.

Copy link
Contributor

mattbennett commented Oct 31, 2018

I have ended up using ServiceRpc and ClusterRpc for the DependencyProviders, and ServiceRpcClient and ClusterRpcClient for the standalone clients. Mostly because we have several test files that import from both modules.

@mattbennett mattbennett requested review from iky and davidszotten Nov 8, 2018

@mattbennett

This comment has been minimized.

Copy link
Contributor

mattbennett commented Nov 12, 2018

@davidszotten are you happy with the changes here now?

@davidszotten

This comment has been minimized.

Copy link
Member

davidszotten commented Nov 18, 2018

this seems fine to me

not sure i'd say a ServiceRpc is a ClusterRpc etc (this inherits atm)

@mattbennett

This comment has been minimized.

Copy link
Contributor

mattbennett commented Nov 18, 2018

not sure i'd say a ServiceRpc is a ClusterRpc etc

Have I said that somewhere?

@davidszotten

This comment has been minimized.

Copy link
Member

davidszotten commented Nov 18, 2018

It’s defined as a subclass

@mattbennett

This comment has been minimized.

Copy link
Contributor

mattbennett commented Nov 19, 2018

Sorry, still confused. ServiceRpc is a pre-targetted ClusterRpc, which happens to be implemented as a subclass. Have I said something to the contrary somewhere?

@davidszotten

This comment has been minimized.

Copy link
Member

davidszotten commented Nov 19, 2018

sorry for being unclear. it's the fact that it's a subclass (="is a") that i'm (mildly) poking at

@mattbennett

This comment has been minimized.

Copy link
Contributor

mattbennett commented Nov 19, 2018

Oh I'm with you. I guess it would be nicer to use composition rather than inheritance, but I don't think this is the first use of inheritance in Nameko that isn't strictly a "is-a" type relationship.

I will merge this and cut another pre-release for the 3x branch.

@mattbennett mattbennett merged commit 216848d into nameko:v3.0.0-rc Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mattbennett mattbennett deleted the mattbennett:cluster-rpc-proxy-dp branch Nov 19, 2018

@davidszotten

This comment has been minimized.

Copy link
Member

davidszotten commented Nov 19, 2018

Indeed. Hence only mildly prodding at this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment