gRPC convergence #508
gRPC convergence #508
Conversation
jskeet
added
the
do not merge
google-cla
bot
added
the
cla: yes
| { | ||
| private const string GrpcCoreAssemblyName = "Grpc.Core, Version=2.0.0.0, Culture=neutral, PublicKeyToken=d754f35622e28bad"; | ||
|
|
||
| // Option names, copied from ChannelOptions |
There was a problem hiding this comment.
It's a little weird that we have these harcoded here (ultimately a dependency). I don't mind at all tbh, it just jumped at me a little be.
I know it will be more reflection, but maybe use the Grpc.Core.ChannelOptions class with all the constants? That way, in the unlikely case those change, we don't have to change the code.
There was a problem hiding this comment.
I'd prefer not to - to be honest, I think it's more likely that the names in the C# code will change e.g. in a new major version than that the underlying values will change, given that the whole ecosystem will depend on those.
| // but in those environments, it's unlikely that Grpc.Core is being used anyway. | ||
| // The other methods can be invoked with delegates, but delegates can't refer directly to methods. | ||
|
|
||
| // TODO: Somehow cache the result of ConvertOptions? It's probably not called terribly frequently. |
There was a problem hiding this comment.
- Is ChannelOption inmutable?
- Consider caching each individual option instead of the list? And you can pick and choose based on the value?
There was a problem hiding this comment.
Yes, both ChannelOption and GrpcChanneloptions are immutable. Caching individual options would be trickier, given that we still need to come up with a sequence of the results. I suspect it's probably simpler to do all or nothing (and let's do nothing until we know we need to do anything...)
| { | ||
| try | ||
| { | ||
| GrpcChannel.ForAddress("https://ignored.com"); |
There was a problem hiding this comment.
If this succeeds, can we guarantee that Grpc.Net.Client is fully usable?
There was a problem hiding this comment.
That's something we need to check with Jan. But I'm pretty sure we'll want some what of forcing it one way or another globally - an environment variable being the simplest approach.
jskeet
force-pushed
the
grpc-convergence
branch
from
0b07692
to
4c94fa6
Compare
…mmits In reality we probably want to bring this code into Google.Api.Gax.Grpc, but that will be a later step.
…ient The adapters will be reimplemented within Google.Api.Gax.Grpc in a later commit (We could do it all in one commit, but this is at least simpler to follow.)
jskeet
force-pushed
the
grpc-convergence
branch
2 times, most recently
from
1fc26ad
to
ee95dc1
Compare
jskeet
removed
the
do not merge
|
This is now ready for real review with an eye to merging, but before I merge I'll create new issues for:
|
|
(I'll then probably try to get the Grpc.Gcp code in before addressing those issues.) |
| protected override ChannelBase CreateChannelImpl(string endpoint, ChannelCredentials credentials, GrpcChannelOptions options) => | ||
| channelFactory.Value.Invoke(endpoint, credentials, options); | ||
|
|
||
| private static Func<string, ChannelCredentials, GrpcChannelOptions, ChannelBase> CreateFactory() |
There was a problem hiding this comment.
tiny nit: CreateChannelFactory
And also, I think a comment here with something like "A function is created, instead of just having a factory method, to avoid the reflection bits/expression building every time."
We take a dependency on Grpc.Net.Client unconditionally, but no dependency on Grpc.Core. GrpcCoreAdapter loads dependencies via reflection (but in a way that is cheap after initial setup), allowing it to be used by applications that have a dependency - which could be conditional based on platform, for example.
Note that GrpcNetClientAdapter ignores the primary user agent option, so we can't easily test that the options are actually used.
This (lazily, and once) checks whether Grpc.Net.Client works - i.e. whether a channel can be constucted - and uses GrpcNetClientAdapter if so. Otherwise, it uses GrpcCoreAdapter. We may well want to tweak the details of this later.
jskeet
force-pushed
the
grpc-convergence
branch
from
ee95dc1
to
338fcd7
Compare
This feels a little too easy to be "it", and there are some aspects we need to look more closely at:
However, this is a good starting point for discussion.
cc @jtattermusch