Adds Grpc.newManagedChannel(String, ChannelCredentials, NameResolverR… #11901
Conversation
| Assume.assumeTrue(Utils.isEpollAvailable()); | ||
| ManagedChannelBuilder<?> managedChannelBuilder | ||
| = Grpc.newChannelBuilderForNameResolverRegistry("unix:///sock.sock", | ||
| InsecureChannelCredentials.create(), NameResolverRegistry.getDefaultRegistry()); |
There was a problem hiding this comment.
This isn't testing that the NameResolverRegistry is actually being used. getDefaultRegistry() is already the default. We need to pass a NameResolverRegistry that clearly behaves differently than the default.
I suggest you create a new registry and add a io.grpc.internal.testing.FakeNameResolverProvider to it. The target string and socket address passed to the fake provider don't matter much; you'd just have to pass the same target string to newChannelBuilderForNameResolverRegistry() and the socket address wouldn't be used. If you follow this existing test, then the authority would become "fake-authority."
2. removes visibleForTesting 3. improves the test case
| assertThat(managedChannelBuilder).isNotNull(); | ||
| ManagedChannel channel = managedChannelBuilder.build(); | ||
| assertThat(channel).isNotNull(); | ||
| assertThat(channel.authority()).isEqualTo("/sock.sock"); |
There was a problem hiding this comment.
The test isn't actually working. FakeNameResolverProvider hard-codes the authority to "fake-authority". So this should be failing, because it isn't actually using your NameResolverRegistry.
And that's because the name resolver registry isn't being passed to the ManagedChannelBuilder.
And looking deeper, that's because the NameResolverRegistry code in ManagedChannelRegistry is a bit of a hack; it doesn't coordinate with the ManagedChannelBuilder right now and instead just assumes some later logic in ManagedChannelImpl will produce similar results. It should ideally select the NameResolverProvider and pass it to the ManagedChannelProvider. But I'm not sure how much of that we want to clean up now.
There was a problem hiding this comment.
I think we will need to change the arguments of ManagedChannelProvider.newChannelBuilder(String, ChannelCredentials). That's a bit annoying though, because we can't make that change atomically everywhere (some implementations aren't in this repo) and this is needing to modify an existing code path. So we need to add a new method to ManagedChannelProvider and have it call the existing method by default. We can update all the ManagedChannelProviders in this repo at the same time (and have the old method call the new method). After this change is imported to Google, we'll update the remaining usages and then we can delete the old method.
But I need to think more about what the new signature should look like. There's some interplay between ManagedChannelRegistry and ManagedChannelImplBuilder.getNameResolverProvider(), and it'd be nice to avoid one side assuming the behavior of the other. I also need to figure out whether the NameResolverRegistry will propagate through the channel to LoadBalancer.Helper.createResolvingOobChannel() and getNameResolverRegistry().
Fixes #11055
Exposes a new method for channel creation which accepts NameResolverRegistry