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

L57 - C# 2.x release #154

Merged
merged 18 commits into from
Jul 30, 2019
Merged

L57 - C# 2.x release #154

merged 18 commits into from
Jul 30, 2019

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Jul 12, 2019

CC @JamesNK @JunTaoLuo feel free to review

@jskeet can you take a look and confirm that this proposal is OK from perspective of client libraries?

@jtattermusch jtattermusch changed the title L56 - C# 2.x release L57 - C# 2.x release Jul 12, 2019
@jtattermusch
Copy link
Contributor Author

CC @apolcyn

@jskeet
Copy link

jskeet commented Jul 13, 2019

It's "okay" for client libraries in terms of "if there's going to be a major version bump, this is a reasonable way of doing it". Obviously it's still going to cause us a certain amount of pain - we may stay on 1.22 for a while, as there are other reasons to take a new major version from the client libraries perspective.

A couple of questions:

  • Why 2.23.0 rather than 2.0.0? I assume it's to keep in line with the other stacks, but it may be worth calling that out explicitly
  • How much work has been done thinking about switching between the two different implementations, and how this might work from a client perspective? For example, if we wanted to enable a scenario where the GCP client libraries could be used with the MS implementation, without the rather large C-Core implementation being present at execution-time at all, how feasible is that? The ChannelBase part feels like the start of that, but unless this has really been gone through in a lot of detail, I can imagine issues. A major version is an opportunity to get that right.

@JamesNK
Copy link
Member

JamesNK commented Jul 13, 2019

For example, if we wanted to enable a scenario where the GCP client libraries could be used with the MS implementation

What APIs do the GCP client libraries use? ChannelCredentials might still be a blocker. Grpc.Auth for example has references to ChannelCredentials and SslCredentials.

@jskeet
Copy link

jskeet commented Jul 13, 2019

I'll have to look for details, but at least:

  • Channel
  • CallOptions
  • ChannelCredentials
  • The streaming types

It may be an infeasible idea, but it would be nice to investigate, and now's the time.

L57-csharp-new-major-version.md Outdated Show resolved Hide resolved
L57-csharp-new-major-version.md Outdated Show resolved Hide resolved
@jtattermusch
Copy link
Contributor Author

It's "okay" for client libraries in terms of "if there's going to be a major version bump, this is a reasonable way of doing it". Obviously it's still going to cause us a certain amount of pain - we may stay on 1.22 for a while, as there are other reasons to take a new major version from the client libraries perspective.

It's fine to stay on 1.22 for a while (and we can backport important backfixes into branch v1.22.x for some time), but we should set some timeline too. (we can sync up offline)
Also, as soon as the v2.23.0 prerelease is out, we should do some testing to make sure things are working as expected (or we can test using nightly nugets https://github.com/grpc/grpc/tree/master/src/csharp#nuget-development-feed-nightly-builds as soon as the changes get merged)
Btw, because client libraries are using Grpc, if you stay on 1.22, you might find yourself in the situation that users who are already on the newest BCL won't be able to use the client libraries.

A couple of questions:

  • Why 2.23.0 rather than 2.0.0? I assume it's to keep in line with the other stacks, but it may be worth calling that out explicitly

You're right, I'll add a note in the proposal.

  • How much work has been done thinking about switching between the two different implementations, and how this might work from a client perspective? For example, if we wanted to enable a scenario where the GCP client libraries could be used with the MS implementation, without the rather large C-Core implementation being present at execution-time at all, how feasible is that? The ChannelBase part feels like the start of that, but unless this has really been gone through in a lot of detail, I can imagine issues. A major version is an opportunity to get that right.

@JamesNK and I spent a significant amount of time trying to come up with a good design for the client side and we investigated some possible approaches (e.g 1. not having a channel concept in grpc-dotnet at all and relying on LiteClientBase as base class for client stubs; 2. trying to migrate all the existing Channel API over to the shared API package, which turned out very troublesome 3. using the ChannelBase concept). The ChannelBase concept seemed to be the best option.

GCP libraries could work with grpc-dotnet client assuming you're not relying on advanced channel functionality (connectivity API, reconnection spec, loadbalancing, service config etc. - these are currently impossible to implement with http client). What's missing:

  • auth integration (what's in Grpc.Auth). This is certainly doable but requires some glue code (Google.Apis.Auth use in grpc-dotnet client) that has not been written yet.
  • note that the concept of ChannelCredentials in not supported by grpc-dotnet at all, but for GCP client libraries, you only need to establish a secure HTTP2 connection to google backends (which grpc-dotnet is fully capable of on Linux and Windows, but not on mac AFAIR due to some openssl limitation) and make sure the right auth headers are populated (doable relatively easily by using Google.Apis.Auth).

@jtattermusch
Copy link
Contributor Author

I think running GCP client libraries on top of grpc-dotnet client is entirely feasible, at least for some APIs (described some challenges in my previous comment). See inline for the specific concepts you're mentioning

I'll have to look for details, but at least:

  • Channel

Channel concept doesn't exist int grpc-dotnet at this point, but ChannelBase will exist in the future, but the client classes are fully API compatible with Grpc.Core. So the way you create instances of client classes is different, but after that, the behavior should be 100% the same as Grpc.Core

  • CallOptions

CallOptions are fully supported (including CallCredentials), except waitForReady, callFlags and writeOptions (but I don't think you need any of those)

  • ChannelCredentials

The channels are created and configured differently, so ChannelCredentials are not supported by grpc-dotnet. See my previous comment about how the auth integration would work.

  • The streaming types

All types of streaming are fully supported.

It may be an infeasible idea, but it would be nice to investigate, and now's the time.

@jtattermusch
Copy link
Contributor Author

@a11r this should be now ready for review.

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

Successfully merging this pull request may close these issues.

6 participants