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

Support BinderChannelBuilder.forTarget. #8633

Merged
merged 3 commits into from Nov 1, 2021
Merged

Conversation

markb74
Copy link
Contributor

@markb74 markb74 commented Oct 28, 2021

Allows BinderChannel to be used with name resolvers.

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 {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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).

Copy link
Contributor Author

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) {
Copy link
Member

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.

Copy link
Contributor Author

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 {
Copy link
Member

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).

@markb74 markb74 merged commit 14eb3b2 into grpc:master Nov 1, 2021
beatrausch pushed a commit to beatrausch/grpc-java that referenced this pull request Nov 4, 2021
Allows this class to be used with custom name resolvers.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2022
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 this pull request may close these issues.

None yet

2 participants