Conversation

YifeiZhuang

@YifeiZhuangYifeiZhuang requested a review from ejona86 October 4, 2021 18:13

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.

@YifeiZhuangYifeiZhuang requested a review from ejona86 October 7, 2021 19:06
@ejona86

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

@YifeiZhuang

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

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

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
Member 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://.com/grpc/grpc-java/pull/8358/files/8f38f34705f34db27e9103928dc27dd856dc467e#r684332029

@YifeiZhuangYifeiZhuang requested a review from ejona86 October 7, 2021 22:37
@YifeiZhuangYifeiZhuang merged commit a2e2f56 into grpc:master Oct 7, 2021
@YifeiZhuangYifeiZhuang deleted the bootstrap_server branch October 7, 2021 23:17
@github-actions-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2022
Sign up for free to subscribe to this conversation on . Already have an account? Sign in.
None yet
None yet

Successfully merging this pull request may close these issues.