Rename Rpc* to Thrift*

[anuraaga]

Copying into here since I think discussion got lost in the middle of a meta-issue.

From @minwoox

Currently, we use RpcXX classes such as RpcRequest, RpcResponse, RpcService, RpcClient, etc. for Thrift only. Because the name includes Rpc, the users expect that the gRPC module uses the same classes which is not true and they usually end up with a surprise.

I wonder if we have to address this issue which will bring a lot of API changes before 1.0.0. These are some of the ways that I can think of to solve that:

Rename the RpcXX classes to ThriftXX to distinguish Thrift and gRPC. The easiest way. There are not so many changes except just names. Abstract RpcRequest and RpcResponse use them in Thrift and gRPC together. I'm not even sure if it's possible also if it's a good idea. gRPC uses ClientCall, ServerCall and listener for sending request and receiving response. gRPC supports streaming and Thrift doesn't. Because nature is different, it could harm Armeria and make it worse to use. Just leave it as it is since no one complains about it. These are just my improvisatory thought and please give an idea if you have. /cc @anuraaga

[anuraaga]

I have had problems reasoning about these classes many times over the years. I think renaming to Thrift is a good solution since it's unclear these could apply well to other RPC frameworks with their own idioms.

[trustin]

I'm not sure renaming is a scalable approach. What happens if we add support for other RPC protocols, say JSON-RPC, Hessian, RSocket etc? Why not just make RpcRequest/Response a StreamMessage of Objects and make it capable of generalizing streaming RPC messages?

[trustin]

As a stopgap solution, how about checking if RPC decorators are present in pure HTTP clients and raising an exception?

[anuraaga]

gRPC wouldn't benefit from that though - it already has a domain specific layer that works well and I wouldn't recommend anyone to use an Armeria layer using Object as a scapegoat instead of the easier to use model. Converging RpcResponse to a streaming object might help other users but not gRPC users I think.

RpcResponse is IMO a response to the fact that thrift does not have a reasonable domain-specific representation of request/response. Other RPC frameworks might also have this problem, but not all of them. Copy-pasting similar, but possibly somewhat different and more idiomatic, logic to different RPC layers that lack if natively seems more scalable to me.

[anuraaga]

Anyways realize this discussion is probably sort of too late to settle for 1.0 - stopgap sounds ok to me too

[minwoox]

I got an another report from a user who was trying to implement an Authorizer with RpcRequest and RpcResponse for gRPC service. I think we should put the description in Javadoc at least. 🤔

[codefromthecrypt]

https://github.com/openzipkin/brave/pull/999 has a lot of commentary on the tension of http vs rpc

honestly that armeria defines itself in terms of http (despite some cheats :P) it is a very worthwhile discussion. Frankly it might be one of those focus things a well prepared pow-wow could chew on.

[Dogacel]

Is this still the case? Does gRPC not use RpcResponse so I can't use DecoratingRpcService with gRPC methods?

[ikhoon]

We can't use DecoratingRpcService with gRPC services. However, RpcRequest and RpcResponse are used to recode unserialized gRPC messages or exceptions raised at the RPC layer. https://github.com/line/armeria/blob/5991574ff6bda3a79302003ae13b02b631afab90/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcLogUtil.java#L57-L57