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: override bootstrap for xds server #8575

Merged
merged 5 commits into from Oct 7, 2021

Conversation

YifeiZhuang
Copy link
Contributor

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.

"#7819" should be part of the commit message.

@@ -37,9 +36,12 @@
import io.grpc.netty.NettyServerBuilder;
import io.grpc.xds.FilterChainMatchingProtocolNegotiators.FilterChainMatchingNegotiatorServerFactory;
import io.grpc.xds.XdsNameResolverProvider.XdsClientPoolFactory;

Copy link
Member

Choose a reason for hiding this comment

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

Remove extra line

* Allows injecting {@link XdsClientPoolFactory} and/or overriding bootstrap configuration, useful
* for testing.
*/
public XdsServerBuilder xdsClientPoolFactory(@Nullable XdsClientPoolFactory xdsClientPoolFactory,
Copy link
Member

Choose a reason for hiding this comment

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

XdsClientPoolFactory is not public, and we don't want it to be public. We just want the bootstrap. And we should probably put "for test" or the like in the name.

@ejona86
Copy link
Member

ejona86 commented Oct 7, 2021

This PR seems to do essentially nothing. What method is the user intended to call?

@YifeiZhuang
Copy link
Contributor Author

This PR seems to do essentially nothing. What method is the user intended to call?

I was thinking they can override bootstrap for their server:
XdsServerBuilder.forPort().xdsClientPoolFactoryForTest(null, bootstrapOverride).build()

@ejona86
Copy link
Member

ejona86 commented Oct 7, 2021

But they can't call xdsClientPoolFactoryForTest; it is package-private.

We don't want XdsClientPoolFactory part of the public API. We need a new method. Also, a name like "xdsClientPoolFactory" is very poor for the user; we should choose a name that has more meaning to the user.

@YifeiZhuang
Copy link
Contributor Author

But they can't call xdsClientPoolFactoryForTest; it is package-private.

We don't want XdsClientPoolFactory part of the public API. We need a new method. Also, a name like "xdsClientPoolFactory" is very poor for the user; we should choose a name that has more meaning to the user.

Yea, because bootstrap is directly related to xdsClientPoolFactory so i put them together, the main reason I had one method is to prevent the situation overrideBootstrap() called first and then xdsClientPoolFactory() would erase the first call, e.g.

XdsServerBuilder xdsClientPoolFactory(XdsClientPoolFactory xdsClientPoolFactory) {
this.xdsClientPoolFactory = xdsClientPoolFactory;
}

public XdsServerBuilder overrideBootstrapForTest(Map<?,?> rawMap ) {
this.xdsClientPoolFactory.setBootstrapOverride(rawMap);
}

but yea they usually can not call xdsClientPoolFactory because it is package private. Our own tests will have the issue at least. We can sure do some trick to always save the bootstrap, but i don't think that is a clean solution.

* Allows providing bootstrap override, useful for testing.
*/
public XdsServerBuilder overrideBootstrapForTest(Map<String, ?> bootstrapOverride) {
this.xdsClientPoolFactory.setBootstrapOverride(
Copy link
Member

Choose a reason for hiding this comment

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

This is mutating a singleton.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why we can't?

Copy link
Member

Choose a reason for hiding this comment

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

That means it would change channels and other servers. We went through this before on client-side. https://github.com/grpc/grpc-java/pull/8358/files/8f38f34705f34db27e9103928dc27dd856dc467e#r684332029

@YifeiZhuang YifeiZhuang merged commit a2e2f56 into grpc:master Oct 7, 2021
@YifeiZhuang YifeiZhuang deleted the bootstrap_server branch October 7, 2021 23:17
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 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