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
Support BinderChannelBuilder.forTarget. #8633
Conversation
Allows this class to be used with custom name resolvers.
import java.net.URI; | ||
|
||
/** A name resolver to always resolve the given URI into the given address. */ | ||
public final class FakeNameResolverProvider extends NameResolverProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testing project is actually published, so this is pubic API. Either we should mark it experimental, or move it to io.grpc.internal.testing
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah of course, thanks. I've had cause to use something like this myself internally, so marking experimental.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to discuss when you used this internally, as I expect it was probably with a grpc-maintainer hat on (e.g., when writing a transport). It wouldn't be "against the rules" to have this be internal but let some specific tests access it (I expect we're doing that already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, on consideration moving to io.grpc.internal.testing
.
* @param targetUri The URI to resolve when requested. | ||
* @param address The address to return for the target URI. | ||
*/ | ||
public static final void register(String targetUri, SocketAddress address) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should delete this method and instead expose the constructor. This method is a bad pattern. After a test using this is over, it should really deregister the provider. Conceptually (but maybe using @After
):
provider = new FakeNameResolverProvider(URI.create(targetUri), address);
NameResolverRegistry.getDefaultRegistry().register(provider);
try {
...
} finally {
NameResolverRegistry.getDefaultRegistry().deregister(provider);
}
I don't think this class needs to make that try-finally easier, but it shouldn't encourage being used in a register-only fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, fixed.
import java.net.URI; | ||
|
||
/** A name resolver to always resolve the given URI into the given address. */ | ||
public final class FakeNameResolverProvider extends NameResolverProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to discuss when you used this internally, as I expect it was probably with a grpc-maintainer hat on (e.g., when writing a transport). It wouldn't be "against the rules" to have this be internal but let some specific tests access it (I expect we're doing that already).
Allows this class to be used with custom name resolvers.
Allows BinderChannel to be used with name resolvers.