Remove NameResolver.Factory

[ejona86]

NameResolver.Factory is old and was meant to be replaced by NameResolverProvider. I've been slowly trying to remove it, especially ManagedChannelBuilder.nameResolverFactory(). I see evidence new users are using it, so this issue is to be a gathering point for why it is being removed and making sure there are alternatives to the current usages.

Since NameResolverRegistry was added in v1.21, most users shouldn't need the factory. A small number of users may need a "default name resolver override". This would be similar to defaultLoadBalancingPolicy() which was added when we removed loadBalancerFactory(). However, the NameResolver API needs tweaks to support that, where each NameResolver would be for a particular scheme and the NameResolverRegistry would select the appropriate NameResolver (instead of the current "call all the name resolvers in order until one understands the URI").

NameResolver.Factory is used internally in grpc. I don't care much about that, since it does little harm, but I don't expect NameResolver.Factory to ever become stable and it should be removed. It just may go slowly due to priorities.

[ejona86]

Removing ManagedChannelBuilder.nameResolverFactory() is a blocker for #3085.

[lburgazzoli]

I'm trying to find how to replace the use of nameResolverFactory in my code here. It looks like one should use NameResolverRegistry.register to register a custom resolver but it does not seems to work or at least I was unable to make it working.

Is there any migration documentation/example/hint about how to replace the deprecate method with a proper solution ?

[ejona86]

There's no migration guide, since the process is just "make a normal name resolver." That would be one that uses a target string to look up addresses.

You'd start with using the ServiceLoader mechanism described in the NameResolverProvider documentation. If you can't use it because you need some global configuration injected, then you'd use NameResolverRegistry.register(). From there the name resolver needs to get selected for your specific channel via forTarget("etcd:///") (or etcd:).

It looks like you are passing in per-channel information via the factory constructor. That should use the target uri. People abusing nameResolverFactory() to pass in channel-specific configuration is a major reason to mark it deprecated.

I don't see much point to passing the etcd hosts as URIs. Yes, that was necessary for etcd itself for compatibility, but it is strange for a gRPC name resolver. I'd probably just pass the host:post for each server, say, separated by commas. But it's not clear why there are pluggable URIResolverLoaders; is that for using things other than DNS to resolve names?

I'd maybe use a target string akin to: etcd://authority/host1:port,host2:port. That's a different meaning for authority than normal, so you could also do something like etcd:///authority/host1:port,host2:port or etcd:///host1:port,host2:port?authority=$authority.

If you really love your URIs, then you could do something more like etcd:http://host1:port,http://host2:port and pull it out with uri.getSchemeSpecificPart(). URIs allow a lot of things. At the end of the day the objective is to get the per-channel configuration into a single string.

[lburgazzoli]

That implementation is quite old so I don't remember all the reason why it is what it is :) I guess some of the original design goal were to hide internals as much as possible so that we have the URIResolverLoaders. Another reason was that there's no support for dns+srv resolvers in grpc-java (at least #2109 is still not resolved) but I think I can now use the ServiceLoader mechanism to implement that one too.

[lburgazzoli]

@ejona86 I got something working but it is quite ugly as etcd itself as well as any example or other client including the go one use a list of URIs. Wonder having grpc-java supporting something like forTargets("a","b", ...) not an option right ?

[ejona86]

@lburgazzoli, the URL list is probably not a good enough reason, since we know everything except the hostname will be thrown away. In my mind you are free to convert the URLs to a target string with a utility and "clean them up" in the process by stripping the other stuff.

[lburgazzoli]

I'm certainly not advocating to support URLs by default but in mind mind something like:

forTargets("http://host:port", "dns+srv:///_etcd._tcp.acme.com", "dns:///etcd.acme.com:8080");

Then under the hoods, gtpc-java could use the same mechanics as today but applied to multiple target ad finally aggregate the result (this is in fact what my custom name resolver was doing). In my case then I'd only have to provide a NameResolver for the http and dns+srv.

However I've implemented what you suggested by having some logic that deals with encoding/deconding from a a list of URLs to a single target entry so what I'm suggesting is not required by since there's a concept of load balancing in grpc-java, I think having an easy way to define multiple targets would be nice.

[ejona86]

I think having an easy way to define multiple targets would be nice.

As a general purpose mechanism it initially seems generic and useful. But it ends up hard-coding a critical part of service discovery as part of the client, which we would consider a Bad Idea. Basically, primary/secondary nodes are hard-coded. For communicating with etcd itself, that's not as strange because you have to bootstrap yourself somehow. But for communicating with an actual backend that would be very weak.

I'm noticing now that etcd may actually be a poor name to use for this resolver. Normally the etcd resolver would look up results in etcd, not connect to etcd.

For communicating with a low-level service like etcd, I'd normally expect either the (not-supported-in-grpc-java) ip: scheme or dns or dns+srv. ip: supports multiple ip addresses separated by comma. (It's not supported because there's not been any need; generally when an ip is used it's just a single IP and dns: works fine. C core only has it because they used it in their unit tests.) You'd use channelBuilder.overrideAuthority() to allow TLS. Both dns and dns+srv support returning multiple IPs and are centrally configurable. So the only reason I can think of (other than "historical reasons") that you'd prefer to list multiple hosts separately is so they can vary in their authority, but that is pretty weak.

Now, I completely understand you're doing things "the etcd way" and that made sense to etcd given how it evolved. It's just that the difficulties experienced don't seem inherent in any way, so I'd be prone to let this etcd-resolver-that-is-rarely-seen be a bit uglier.

I'll also note that it would be possible to gather the etcd configuration for a set of endpoints and give it a name and use that name for your etcd: target string. So the configuration would be out-of-band to the target string. A layer of indirection and more global state, but depending on the specific use-case it could make sense.

[lburgazzoli]

@ejona86 thx for your advice

[tommyulfsparre]

We have a custom NameResolverProvider that can be configured with a custom domain. To give an example: doing new MyNameResolverProvider("example.com") and using forTarget("my-resolver://foobar") will cause the NameResolver to resolves address with the configured custom domain foobar.example.com.

Instantiate and register our NameResolverProvider into the global registry does not prevent any library code from also doing the same and registering with a higher priority and with a custom domain.

Is there a way for us to not have to depend on the global registry?