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

xds: allow injecting bootstrapOverride in xdsNameResolverProvider #8358

Merged
merged 5 commits into from Aug 11, 2021

Conversation

YifeiZhuang
Copy link
Contributor

@YifeiZhuang YifeiZhuang commented Jul 30, 2021

fix #7819

@YifeiZhuang YifeiZhuang marked this pull request as ready for review July 30, 2021 21:42
/**
* Allows injecting bootstrapOverride to the name resolver.
* */
public XdsNameResolver newNameResolver(URI targetUri, Args args,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who would call this method? I think we want bootstrapOverride to be passed to a constructor/factoryMethod for the provider itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, i didn't test it.
The bootstrapOverride needs to be saved here and then passed to xdsNameResolver when channel creates a new resolver.

authority = GrpcUtil.checkAuthority(checkNotNull(name, "name"));
this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser");
this.syncContext = checkNotNull(syncContext, "syncContext");
this.scheduler = checkNotNull(scheduler, "scheduler");
this.xdsClientPoolFactory = checkNotNull(xdsClientPoolFactory, "xdsClientPoolFactory");
this.xdsClientPoolFactory.setBootstrapOverride(bootstrapOverride);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mutates the default factory, which would impact other xds clients. Create a new instance if bootstrapOverride is set.

public static XdsNameResolverProvider createForTest(String scheme,
@Nullable Map<String, ?> bootstrapOverride) {
XdsNameResolverProvider provider = new XdsNameResolverProvider();
provider.scheme = checkNotNull(scheme, "scheme");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a constructor? That'd allow the fields to be final. You'd need to make a no-arg constructor as well.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a change to restore the static factory method.

*/
public static XdsNameResolverProvider createForTest(String scheme,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had intended for the constructor to be private, and for us to still have the "for test" factory method to discourage it for other uses.

@YifeiZhuang YifeiZhuang merged commit 1eb1d15 into grpc:master Aug 11, 2021
@YifeiZhuang YifeiZhuang deleted the zivy/bootstrap_injection branch August 11, 2021 17:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2021
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.

Unit testing gRPC client/server with xDS?
2 participants