Add C# LiteClientBase and "lite_client" codegen option #18705

jtattermusch · 2019-04-10T06:21:54Z

Based on #18532

To allow creating client stubs while only depending on Grpc.Core.Api (= the managed code).
To make this useable, we would also need to add experimental codegen option to generate the lite stubs in codegen.

Some more context in the doc grpc/grpc-dotnet#180. Note that we haven't figured out many questions around the client-side API (and how to expose the concept of a "client channel", along with all its functionality), so the solution with "LiteClientBase" might be temporary.

jtattermusch · 2019-04-10T06:22:31Z

CC @apolcyn @JamesNK @JunTaoLuo

jtattermusch · 2019-04-10T06:23:08Z

src/csharp/Grpc.Core.Api/LiteClientBase.cs

+    /// is provided by decorating the call invoker.
+    /// Note: experimental API that can change or be removed without any prior notice.
+    /// </summary>
+    public abstract class LiteClientBase


the name of the class is TBD.

CoreClientBase? NetCoreClientBase? ManagedClientBase?

Since the alternative also runs on .NET Core. I prefer ManagedClientbase as suggested in https://github.com/grpc/grpc-dotnet/pull/180/files.

jtattermusch · 2019-04-26T15:10:11Z

@JamesNK @JunTaoLuo I haven't quite figured out the name of the protoc plugin option yet, but the LiteClientBase and protoc plugin changes are mostly done. Worth taking an initial look.

JunTaoLuo · 2019-05-01T21:33:08Z

src/csharp/Grpc.Core.Api/LiteClientBase.cs

+        /// <summary>
+        /// Call invoker that throws <c>NotImplementedException</c> for all requests.
+        /// </summary>
+        private class UnimplementedCallInvoker : CallInvoker


Maybe we can move UnimplementedCallInvoker from Grpc.Core to Grpc.Core.Api and reuse it here instead of creating a private class?

It would then need to be public or internal (there is an internals visible to attribute that gives access to Grpc.Core). I don't think either is worth the hassle compared to some copy and paste.

JunTaoLuo · 2019-05-01T21:43:22Z

Edit: we spoke about this offline and I'm okay with ClientBase and LiteClientBase side by side. However, I think the default should only generate ClientBase so there's no change from previous behaviour. We can independently opt in to LiteClientBase or opt out of ClientBase with options.

It looks like by default, both a ClientBase and a LiteClientBase will be generated. I'm not sure if that's the best option since current client projects will be bloated after upgrading. The fact that no one should ever need to have both ClientBase and LiteClientBase side by side, I would modify the logic so only one of them is generated, not both.

For example, add a --use-lite-client option which defaults to false. This will be an option that's orthogonal to --no-client:

Options	`--no-client`	no `--no-client`
no `--use-lite-client`	do not generate client	generate using ClientBase
`--use-lite-client`	do not generate client, advise `--use-lite-client` will be ignored when `--no-client` is set	generate using LiteClientBase

JamesNK · 2019-05-03T07:01:43Z

src/csharp/Grpc.Core.Api/LiteClientBase.cs

+    /// is provided by decorating the call invoker.
+    /// Note: experimental API that can change or be removed without any prior notice.
+    /// </summary>
+    public abstract class LiteClientBase


CoreClientBase? NetCoreClientBase? ManagedClientBase?

src/csharp/Grpc.Core.Api/LiteClientBase.cs

JamesNK · 2019-05-03T07:08:41Z

src/csharp/Grpc.Core.Api/LiteClientBase.cs

+        /// <summary>
+        /// Call invoker that throws <c>NotImplementedException</c> for all requests.
+        /// </summary>
+        private class UnimplementedCallInvoker : CallInvoker


It would then need to be public or internal (there is an internals visible to attribute that gives access to Grpc.Core). I don't think either is worth the hassle compared to some copy and paste.

src/csharp/Grpc.IntegrationTesting/BenchmarkServiceGrpc.cs

src/compiler/csharp_plugin.cc

jtattermusch · 2019-05-29T18:02:48Z

After some delay, I think this is now ready for final review. Here are some modifications I made:

I added lite_client plugin options (false by default). If true, a client that inherits from LiteClientBase will be generated.
There is no way to generate both "lite" and "full" client at the same time. I gave that a bit more thought and it seemed that would only create confusion (especially around needing to use different client class names when both "full" and "lite" would be both).

Usage scenarios:

Using Grpc.Core only: just use the client generated by default (lite_client=false) and everything will work as always.
Using Grpc.Core and grpc-dotnet in the same application: generate the full client (lite_client=false). All the dependencies will be fine because we're referencing Grpc.Core and the clients will be fully usable with grpc-dotnet (use constructor that takes a CallInvoker).
Using grpc-dotnet only (managed scenario): set lite_client=true and there will be no dependencies on Grpc.Core (only Grpc.Core.Api).

jtattermusch · 2019-05-29T18:03:09Z

@apolcyn @JunTaoLuo @JamesNK PTAL.

JamesNK · 2019-05-29T19:28:51Z

I added lite_client plugin options (false by default)

Who sets the option?

Have you thought about the name anymore? re: #18705 (comment)

apolcyn

general comment: can we generate lite_client protos on one of the examples or tests? it may help for comparing diffs later

apolcyn · 2019-05-29T22:11:48Z

src/compiler/csharp_generator.cc

+    out->Print("public partial class $name$ : grpc::ClientBase<$name$>\n",
+               "name", GetClientClassName(service));
+  } else {
+    out->Print("/// <summary>Lite client for $servicename$</summary>\n",


nit: it may help to add a "experimental API" comment to this generated doc

I added a version of math.proto that uses the lite_client option and I checked in the generated code.

apolcyn

LGTM with two nits. In particular I'd prefer to check in new generated stubs from lite_client mode here

JunTaoLuo

@JamesNK We would most likely set the --lite_client option in a build target in Grpc.AspNetCore.Server.

JamesNK · 2019-05-30T23:48:44Z

We would most likely set the --lite_client option in a build target in Grpc.AspNetCore.Server.

I'm not super familiar with how the proto generator works.

So would Grpc.AspNetCore.Server set some value to say the lite client should be used and then Grpc.Tools would use that value to decide whether to include --lite_client when it invokes the proto generator?

jtattermusch · 2019-05-31T14:18:38Z

We would most likely set the --lite_client option in a build target in Grpc.AspNetCore.Server.

I'm not super familiar with how the proto generator works.

So would Grpc.AspNetCore.Server set some value to say the lite client should be used and then Grpc.Tools would use that value to decide whether to include --lite_client when it invokes the proto generator?

Yes, except to be precise, the option is not passed to the plugin as --lite_client but in a slightly different form, that is based on how options are passed to protoc plugins (they can be added as prefix to --grpc_out or separately under --grpc_opt): https://github.com/protocolbuffers/protobuf/blob/fa5a69e73b0dd667ff15062adbc170310d440ee9/src/google/protobuf/compiler/command_line_interface.h#L174

jtattermusch · 2019-06-03T09:38:33Z

Known failures:
#18416
#18967
#19005

jtattermusch commented Apr 10, 2019

View reviewed changes

jtattermusch force-pushed the csharp_add_lite_client branch from 3e8fc1d to 70a8f5b Compare April 10, 2019 08:55

jtattermusch mentioned this pull request Apr 16, 2019

Move types required for a managed gRPC HttpClient to Grpc.Core.Api grpc/grpc-dotnet#210

Closed

jtattermusch force-pushed the csharp_add_lite_client branch from 70a8f5b to 9f5b894 Compare April 26, 2019 11:40

jtattermusch added lang/C# release notes: yes Indicates if PR needs to be in release notes labels Apr 29, 2019

JunTaoLuo reviewed May 1, 2019

View reviewed changes

JamesNK reviewed May 3, 2019

View reviewed changes

src/csharp/Grpc.IntegrationTesting/BenchmarkServiceGrpc.cs Outdated Show resolved Hide resolved

JunTaoLuo reviewed May 7, 2019

View reviewed changes

src/compiler/csharp_plugin.cc Outdated Show resolved Hide resolved

jtattermusch added 4 commits May 29, 2019 12:50

add LiteClientBase

8cecf60

C# lite client codegen

25e3d26

only generate full or lite client, never both

1b8418b

regenerate C# protos

81a2c84

jtattermusch force-pushed the csharp_add_lite_client branch from 9f5b894 to 81a2c84 Compare May 29, 2019 17:55

jtattermusch changed the title ~~WIP: Add C# LiteClientBase~~ Add C# LiteClientBase and "lite_client" codegen option May 29, 2019

apolcyn reviewed May 29, 2019

View reviewed changes

apolcyn approved these changes May 30, 2019

View reviewed changes

JunTaoLuo approved these changes May 30, 2019

View reviewed changes

jtattermusch added 3 commits May 31, 2019 10:37

also generate code with "lite_client" option set

4a4cf28

add generated code for math_with_protoc_options.proto

8cb8d58

check callInvoker is not null

a432e2f

jtattermusch merged commit be9cb5e into grpc:master Jun 3, 2019

lock bot locked as resolved and limited conversation to collaborators Sep 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C# LiteClientBase and "lite_client" codegen option #18705

Add C# LiteClientBase and "lite_client" codegen option #18705

jtattermusch commented Apr 10, 2019

jtattermusch commented Apr 10, 2019

jtattermusch Apr 10, 2019

JamesNK May 3, 2019

JunTaoLuo May 3, 2019

jtattermusch commented Apr 26, 2019

JunTaoLuo May 1, 2019

JamesNK May 3, 2019

JunTaoLuo commented May 1, 2019 •

edited

JamesNK May 3, 2019

JamesNK May 3, 2019

jtattermusch commented May 29, 2019

jtattermusch commented May 29, 2019

JamesNK commented May 29, 2019

apolcyn left a comment

apolcyn May 29, 2019

jtattermusch May 31, 2019

apolcyn left a comment

JunTaoLuo left a comment

JamesNK commented May 30, 2019 •

edited

jtattermusch commented May 31, 2019

jtattermusch commented Jun 3, 2019

Add C# LiteClientBase and "lite_client" codegen option #18705

Add C# LiteClientBase and "lite_client" codegen option #18705

Conversation

jtattermusch commented Apr 10, 2019

jtattermusch commented Apr 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch commented Apr 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented May 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch commented May 29, 2019

jtattermusch commented May 29, 2019

JamesNK commented May 29, 2019

apolcyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apolcyn left a comment

Choose a reason for hiding this comment

JunTaoLuo left a comment

Choose a reason for hiding this comment

JamesNK commented May 30, 2019 • edited

jtattermusch commented May 31, 2019

jtattermusch commented Jun 3, 2019

JunTaoLuo commented May 1, 2019 •

edited

JamesNK commented May 30, 2019 •

edited