Make adding a new RPC to a service non-breaking for generated C##5928
Make adding a new RPC to a service non-breaking for generated C##5928jtattermusch merged 20 commits intogrpc:masterfrom
Conversation
|
@jskeet @chrisdunelm I think this is ready for review. |
jtattermusch
changed the title
|
Am I right in saying that if we didn't need the |
|
Basically |
|
I definitely prefer WithHost over the mutable approach before :) If you think we need to ClientBase anyway, that's reasonable. In our generated code we have a separation of an abstract base class and then a concrete subclass, rather than your approach here with channel/call invoker - but I think each makes sense in its own context. Note that I've only looked at the generated code, and none of the C++ - I assume that's the intention? |
|
(I am mostly trying to first figure out the right C# API here, then I can get the rest of the PR reviewed by folks on the gRPC team.) I could have a separate abstract class and concrete subclass, but because they both need to inherit from ClientBase, they would essentially look the same, so such distinction wouldn't buy us much here (I actually had two classes in earlier version of this PR and decided to merge them into one). Btw, in earlier versions of the PR, I also didn't have the ClientBaseConfiguration concept - basically the WithHost (and others) were implemented by just adding a decorator over the existing callInvoker (and I only needed the |
|
Right. The ClientBase definitely affects things - in our case the concrete class is a subclass of the abstract class, and the abstract class just throws exceptions. As no base class is involved, there's no state at all. Will have one more look at this, but I think it's looking good - it certainly solves the versioning problem we had before. |
There was a problem hiding this comment.
I don't think the range is required here - I believe you can just leave it as 2015.
There was a problem hiding this comment.
That's required by our copyright checker - it sees the file has been touched in git history in 2016 and demands updating the copyright.
|
Okay, basically happy - worth waiting for Chris to have a careful look too though, I think. |
|
FYI I've built protoc.exe and grpc_csharp_plugin.exe from commit ee908f5555 (current head of this PR) using our Jenkins artifact builder. You can download it here: or you can use a dev version of Grpc.Tools nuget package here: |
|
Apologies, but I'm not going to be able to take a detailed look at this until next Tuesday (due to UK Easter public holidays). Generally I think it looks OK, although I can't quite see the need for some of the complexity at the moment - I'll spend more time looking at it as soon as I can. |
jtattermusch
force-pushed
the
csharp_clientside_abstractclass
branch
from
ee908f5 to
09e1688
Compare
jtattermusch
changed the title
|
Rebased this PR and fixed some copyrights. Also, instead of completely removing the client-side interface I marked it as obsolete (I've done the same for the server-side for now). Removing both interfaces (IMathClient and IMath) in a followup pull request will be trivial. |
jtattermusch
force-pushed
the
csharp_clientside_abstractclass
branch
from
09e1688 to
5d27030
Compare
|
@stanley-cheung friendly ping |
|
LGTM I don't have much to add. Coincidentally we have the same |
5 participants
Based on #5751.
The intention is to fix #5416, but also fixes #5323 along the way and addresses some other concerns.
Note: below I am talking about "MathClient" for simplicity, but everything is meant in general for all client stubs.
As suggested in #5416, I initially wanted to generate two classes - the "AbstractMathClient" and "MathClient", but that ended up not making much sense: It seems reasonable that client stubs would inherit from a base class (here ClientBase) to be able to establish some common ground for all client classes (and to be able to add some general functionality to all clients in the future). But with that logic, both "AbstractMathClient" and "MathClient" would need to inherit from the same class (and define the right constructors), which in the end would not make "AbstractMathClient" any more lightweight (for testing etc.) than "MathClient". So currently there's only one class "MathClient" that can be used in all situations - as a real client and for creating test doubles, if needed.
Having only a single "MathClient" class, I needed to get rid of the tight coupling between MathClient and a "real" Channel. I achieved that by introducing a concept of "CallInvoker" - an abstract class that allows invoking calls (DefaultCallInvoker is the default implementation that starts calls using a real Channel).
When defining custom test doubles (by using a mocking framework or by manually inheriting from MathClient), the CallInvoker concept is used to make all invocations just throw NotImplementedException (which basically gives us the functionality we would normally expect from AbstractMathClient).
The CallInvoker concept also allows intercepting of the calls - I used the opportunity and refactored the ClientBase a bit to allow a cleaner (and thread safe) approach to override a "host" for the client. Same approach can be used to intercept the fields in CallOptions (e.g. to provide a per-client default timeout or a header interceptor). See the "ClientBase.WithHost" method to see how client stubs are configured now.
Finally, breaking the coupling between client and channel theoretically allows implementation of a testing in-process client-server pair that bypasses C core (which is something we've discussed in the past and it seems to offer some benefits). It is still by no means clear to me that this is something we actually want to do, but this PR at least brings an API that would allow this at all.
There are two ways in which this change is breaking:
Sorry that this change had to be so big, I didn't really find a good way to split it up. It might make sense to review some changes commit-by-commit (the last commit contains the regenerated code).