xds: allow injecting bootstrapOverride in xdsNameResolverProvider#8358
xds: allow injecting bootstrapOverride in xdsNameResolverProvider#8358YifeiZhuang merged 5 commits intogrpc:masterfrom
Conversation
| /** | ||
| * Allows injecting bootstrapOverride to the name resolver. | ||
| * */ | ||
| public XdsNameResolver newNameResolver(URI targetUri, Args args, |
There was a problem hiding this comment.
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.
Right, i didn't test it.
The bootstrapOverride needs to be saved here and then passed to xdsNameResolver when channel creates a new resolver.
| 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.
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.
Add a constructor? That'd allow the fields to be final. You'd need to make a no-arg constructor as well.
ejona86
left a comment
There was a problem hiding this comment.
Just a change to restore the static factory method.
| * A convenient constructor to allow creating a {@link XdsNameResolverProvider} with custom scheme | ||
| * and bootstrap. | ||
| */ | ||
| public static XdsNameResolverProvider createForTest(String scheme, |
There was a problem hiding this comment.
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