-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
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.
"#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; | |||
|
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.
Remove extra line
* Allows injecting {@link XdsClientPoolFactory} and/or overriding bootstrap configuration, useful | ||
* for testing. | ||
*/ | ||
public XdsServerBuilder xdsClientPoolFactory(@Nullable XdsClientPoolFactory xdsClientPoolFactory, |
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.
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.
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: |
But they can't call 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
but yea they usually can not call |
* Allows providing bootstrap override, useful for testing. | ||
*/ | ||
public XdsServerBuilder overrideBootstrapForTest(Map<String, ?> bootstrapOverride) { | ||
this.xdsClientPoolFactory.setBootstrapOverride( |
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 is mutating a singleton.
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.
why we can't?
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.
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
#7819