Skip to content
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

yarpc module is called modules/rpc #267

Closed
yichengq opened this issue Feb 16, 2017 · 13 comments
Closed

yarpc module is called modules/rpc #267

yichengq opened this issue Feb 16, 2017 · 13 comments

Comments

@yichengq
Copy link
Contributor

IIRC, yarpc is just one type of RPC system which could be supported as modules. Similarly, gRPC may be supported too in the future.

@ascandella
Copy link
Contributor

ascandella commented Feb 16, 2017 via email

@yichengq
Copy link
Contributor Author

My point is that YARPC uses the general name rpc, which is not straightforward.
There could be xRPC, yRPC, zRPC in the future, and they would all be options/choices for the developer.

@HelloGrayson
Copy link
Contributor

Sure, using yarpc as a key might be the least surprising - in fact, its not a terrible idea.

On the other hand, UberFX shouldn't need to support another messaging platform since YARPC can plug in transports like tchannel, http, gRPC, and Cherami. Customers have expressed interest for Kafka, QUIC, and reactive sockets as well.

I'm flexible here - whether the key is called rpc or yarpc, UberFX shouldn't have to make room for an additional RPC system since that is the exact goal of YARPC.

Perhaps the biggest argument for calling the key "yarpc" is to be explicit, and tell the use exactly what their getting.

@sectioneight your call.

@ascandella
Copy link
Contributor

UberFx will not support any rpc modules other than YARPC. Its primary goal is to make Uber service development simple and sane, while reducing confusion. If we end up needing to use xRPC in the future, then the yarpc ecosystem will need to be extended to support xRPC -- we're not going to ask services owners to choose between yarpc and xRPC.

@ascandella
Copy link
Contributor

@uber-go/service-framework thoughts?

@yichengq
Copy link
Contributor Author

Understand. Thanks for the answer!

@madhuravi
Copy link
Contributor

I can go either way but there doesn't seem to be a strong reason to change given future implementations will go under the same umbrella.

Everyone seems to be on the same page, resolving.

@bufdev
Copy link
Contributor

bufdev commented Feb 23, 2017

@sectioneight you can quickly re-close this if it's decided and I can't convince everyone, but my opinion re: #315

FX's primary purpose is to make Uber service development simpler, but that doesn't mean that FX can't have implications as a general service framework for the rest of the golang community. If it is sufficiently low-cost, I think FX should aim to support use cases that aren't Uber-specific, and I think this is one of them. YARPC may support gRPC soon, but if I was a third-party developer, and I had the choice of using a new rpc library that supported gRPC, or github.com/grpc/grpc-go directly, I would probably want to use the official gRPC golang library right now. I think renaming from rpc to yarpc is a good thing to do for use of FX outside of Uber, and is a low-cost choice.

@breerly

@bufdev bufdev reopened this Feb 23, 2017
@bufdev
Copy link
Contributor

bufdev commented Feb 23, 2017

Also note that the http module is uhttp, not http.

@HelloGrayson
Copy link
Contributor

I think we should rename the module key to yarpc.

Not because I think we should make room for another RPC system (because we shouldn't be introducing additional RPC systems when we can already plug those transparently into YARPC - e.g. YARPC gives us a single way to write handlers and outbound calls).

The reason I think we should rename the key is to make it obvious and explicit what is being configured - in this case, they are configuring YARPC.

I much prefer:

modules:
  yarpc:
    ...

@bufdev
Copy link
Contributor

bufdev commented Feb 23, 2017

Note I think this should mean that the package should move to be yarpc as well, re convo with @madhuravi about how default keys should match the package name.

@ascandella
Copy link
Contributor

ascandella commented Feb 23, 2017

uhttp is to avoid clashing with the stdlib http. i'm fine with renaming the module to yarpc.

ascandella added a commit that referenced this issue Feb 23, 2017
@madhuravi
Copy link
Contributor

Aiden, I think Peter is referring to a conversation we had earlier today. It's similar to Grayson's sentiments above. Our config yaml keys should match the package names so it's intuitive for users.

So in yaml we would now have:

modules:
yarpc:
...
uhttp:
...

So it's clear what the config keys stand for. Our config keys for http don't match the module name, we can change that.

ascandella added a commit that referenced this issue Mar 1, 2017
ascandella pushed a commit that referenced this issue Mar 2, 2017
* Rename RPC module to YARPC

Fixes #267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants