-
Notifications
You must be signed in to change notification settings - Fork 3.9k
xds: allow injecting bootstrapOverride in xdsNameResolverProvider #8358
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
Conversation
/** | ||
* Allows injecting bootstrapOverride to the name resolver. | ||
* */ | ||
public XdsNameResolver newNameResolver(URI targetUri, Args args, |
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.
Who would call this method? I think we want bootstrapOverride
to be passed to a constructor/factoryMethod for the provider itself.
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.
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); |
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.
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"); |
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.
Add a constructor? That'd allow the fields to be final. You'd need to make a no-arg constructor as well.
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.
Just a change to restore the static factory method.
*/ | ||
public static XdsNameResolverProvider createForTest(String scheme, |
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 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.
fix #7819