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

Allow choosing the ManagedChannelProvider based on the NameResolver #3085

Closed
carl-mastrangelo opened this issue Jun 9, 2017 · 13 comments · Fixed by #9076
Closed

Allow choosing the ManagedChannelProvider based on the NameResolver #3085

carl-mastrangelo opened this issue Jun 9, 2017 · 13 comments · Fixed by #9076
Milestone

Comments

@carl-mastrangelo
Copy link
Contributor

ManagedChannelBuilder.forTarget knows the scheme of the target, but doesn't pass it to the ManagedChannelProvider. This makes it hard for the service loader to pick a proper Provider, because it may pick incorrectly.

Consider passing the scheme to the MCP, so that it can decide whether or not its available.

@ejona86
Copy link
Member

ejona86 commented Jun 12, 2017

I don't understand how you've been led to think this. ManagedChannelBuilder.forTarget immediately turns into ManagedChannelProvider.builderForTarget. ManagedChannelBuilder basically knows nothing and lets ManagedChannelProvider do all the work.

In addition, we don't actually use the scheme in the channel at all; NameResolver.Factory instances are the only thing making use of the scheme today. While I'd like to change that so that we choose the NameResolver.Factory based on the scheme, it's still name resolver logic.

If this is so that ManagedChannelProvider can choose a compatible provider, then I've been considering allowing the NameResolver and ManagedChannelProvider to return a Collection<Class<? extends SocketAddress>> to coordinate selecting a compatible channel. There could be a InProcessNameResolver that returns InProcessSocketAddress or a UnixNameResolver that returns Netty's DomainSocketAddress.

@carl-mastrangelo
Copy link
Contributor Author

Channel builders may wish to exclude themselves from the running if they know they cannot support certain names. For example, if the target passed was uds:///var/grpc.socket, OkHTTP would not be an appropriate transport. It cannot exclude itself except via priority, which is kind of hacky.

Both priority and isAvailable try to return the best answer, but they really don't know what they are signing up for. MCB knows only the target being connected to, so its the only thing the Provider has to go on.

@ejona86
Copy link
Member

ejona86 commented Jun 12, 2017

The transport should not hard-code a white/black list of schemes/name resolvers; that assumes you know all name resolvers and how they behave.

My suggestion about having a Collection<Class<? extends SocketAddress>> negotiation seems like it is for exactly the use case you mentioned. Since OkHttp wouldn't support DomainSocketAddress it would be skipped for a uds name resolver.

@ejona86 ejona86 changed the title Consider passing scheme to ManagedChannelProvider Allow choosing the ManagedChannelProvider based on the NameResolver Nov 8, 2017
@ejona86 ejona86 added this to the Next milestone Nov 8, 2017
@carl-mastrangelo
Copy link
Contributor Author

carl-mastrangelo commented Aug 17, 2018

Thinking about this some, there is some complication where the API doesn't work out so great. NameResolvers (in my branch) produce a Set<? extends Class<? extends SocketAddress>>. This set contains all the possible socketAddress types that could be returned. The default implementation just returns a singleton set of SocketAddress, implying no restriction. This is overridden by each NameResolver.Factory.

The immediate issue is that when trying to pick a NameResolver, each one could return several types of Addresses which could intersect the Channel supported types. It isn't clear how to rank these intersections and pick a name resolver. On the flip side, there may be multiple kinds of channels, each which consume different kinds sockets. If resolver 1 can produce socket addresses A+B, resolver 2 can produce socket addresses B+C+D, and channel 1 accepts sockets of type B+C, and channel 2 accepts sockets of type A+D, it isn't clear which resolver to use. Channel 1 and resolver 1 could be the higher priority, but an error could occur when the resolver produces an A. The problem exists too when switching to resolver 2 which might produce a D, still causing an error.

Maybe I'm overthinking it, but I think we need to know the Socket type before we can pick ClientTransportFactory (oh yeah, Netty might pick a different transport type based on the address, such as Nio or Epoll, a decision which is made at channel build, not at transport creation). Maybe it's good enough if there is any intersection. To support this, the NR could say what kind of SocketAddress would be produced for a given name, and then see if it works with the channels in priority order. Roughly:

for c in channels:  
  for nrf in nameresolverfactories:
     if c.canaccept(nrf.sockettype(target))
       break success

@ejona86 WDYT?

@ejona86
Copy link
Member

ejona86 commented Aug 17, 2018

It isn't clear how to rank these intersections and pick a name resolver.

Don't choose a name resolver by this mechanism. Name resolvers should still be selected by their scheme+priority. Instead, the chosen name resolver should influence which transport is chosen.

If resolver 1 can produce socket addresses A+B, resolver 2 can produce socket addresses B+C+D, and channel 1 accepts sockets of type B+C, and channel 2 accepts sockets of type A+D, it isn't clear which resolver to use.

If resolver 1 was selected, then there is no compatible channel. If resolver 2 was selected, there is no compatible channel. The channel needs to support all possible return types, because it doesn't know which one it will get.

@carl-mastrangelo
Copy link
Contributor Author

If resolver 1 was selected, then there is no compatible channel. If resolver 2 was selected, there is no compatible channel. The channel needs to support all possible return types, because it doesn't know which one it will get.

The compatibility is not clear here. A NR may overwhelmingly return one type of socket address vs. another, so it would be surprising to a user who see an error on channel creation. Resolver 1 could produce InetSocketAddresses or LocalAddresses. The user may be passing in only domain names, which would mean the second kind would never be returned in reality, but it would preclude OkHTTP from being supported.

Likewise, if we made a super NameResolver that could return Inprocess, Inet, and Unix domain addresses, no channel would support all of them. Forcing the Channel to support all possible addresses, would also prevent adding support for NRs for new socket types, for fear of breaking channel build.

@ejona86
Copy link
Member

ejona86 commented Aug 17, 2018

A NR may overwhelmingly return one type of socket address vs. another, so it would be surprising to a user who see an error on channel creation.

Not really. But let's say it is surprising. Wouldn't it be even more surprising if you're in production and then all of a sudden your application is broken because the NR started returning different results?

Likewise, if we made a super NameResolver that could return Inprocess, Inet, and Unix domain addresses, no channel would support all of them.

Don't do that. Easy. "Quit hitting yourself."

In general, I would expect a NR to only return one type. It's unclear whether we consider IPv4 and IPv6 as two types, but that's the sort of differences I'm expecting from a NR.

@carl-mastrangelo
Copy link
Contributor Author

Not really. But let's say it is surprising. Wouldn't it be even more surprising if you're in production and then all of a sudden your application is broken because the NR started returning different results?

This is very much the case. Consider a hypothetical NettyResolver which can resolve DNS, and Unix Addresses. The NettyClientTransport may only support DNS because Epoll isn't available, but the NR doesn't know that. We would end up not using the NettyName Resolver with the Netty Channel. That's wrong. (and, forcing the NR to know about all supported transports to say which sockets can be resolved is wrong).

Don't do that. Easy. "Quit hitting yourself."

Not so easy. We would start getting into special casing about what is a socket address anyways? For example, DnsNameResolver can return one of two types of addresses. (when you go look you'll say "oh well that's different", but try expressing that in the javadoc)

@ejona86
Copy link
Member

ejona86 commented Aug 17, 2018

Consider a hypothetical

I'm going to stop you there. Why is this name resolver wanting to do that? Is it essential that we support that use case?

If you feel that it is essential to support, then it seems it requires that the NameResolver do I/O before address negotiation can take place. That would require us to re-architect ManagedChannelBuilder and likely move ManagedChannelImpl into io.grpc. While maybe an option, I would expect a really strong reason as to why it was necessary.

@carl-mastrangelo
Copy link
Contributor Author

I'm going to stop you there. Why is this name resolver wanting to do that? Is it essential that we support that use case?

To do automatically do the right thing? To pass a single "target" string and gRPC figure out how to construct a channel that just works. Otherwise, we might as well not have ManagedChannelProvider or NameResolverProvider at all and force people to be up front about how they want it to work.

Doing a half hearted automatic channel construction is worse than not at all. Seeing as #4750 clearly needs to be spec'd out more clearly, I'm going to unassign myself.

@idelvall
Copy link
Contributor

idelvall commented Jan 17, 2019

This is very much the case. Consider a hypothetical NettyResolver which can resolve DNS, and Unix Addresses. The NettyClientTransport may only support DNS because Epoll isn't available, but the NR doesn't know that. We would end up not using the NettyName Resolver with the Netty Channel. That's wrong. (and, forcing the NR to know about all supported transports to say which sockets can be resolved is wrong).

From my perspective, the root of the problem is that both, channels and NRs, are loaded via service loaders. This means that they are loaded in a complete independent way, when the reality is that, as you pointed out @carl-mastrangelo, there exist certain entanglement between them.

Maybe i am missing something, but wouldn't it be simpler if NR providers don't exist at all and only explicit NR factories are used?

I mean, channel implementations could define a default NR factory by themselves, one granted to be coherent with their supported address types, and on the other hand application developers already can specify a different NR factory instance if they are interested in doing so.

So why NR providers exist?

@ST-DDT
Copy link
Contributor

ST-DDT commented Oct 4, 2019

Channel type selection (in-process/netty) via NameResolvers or a similar class would be a very nice feature for config based application setups. E.g. use in-process for tests and netty for production without changing a single line of code.

@dapengzhang0
Copy link
Member

@ST-DDT NameResolver should only be able to distinguish target types (e.g. in-process or not in-process), but should not distinguish transport types (e.g. netty or okhttp).

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

Successfully merging a pull request may close this issue.

5 participants