-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: decouple xds channel creation and bootstrapping #7764
xds: decouple xds channel creation and bootstrapping #7764
Conversation
…arguements for XdsClient creations.
…sue of xDS channel.
@@ -121,26 +134,27 @@ public XdsClient returnObject(Object object) { | |||
if (refCount == 0) { | |||
xdsClient.shutdown(); | |||
xdsClient = null; | |||
channel.shutdownNow(); |
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.
It can be shutdown more gracefully with shutdown()
. As long as all xdsClients cancel their streams in xdsClient.shutdown()
, the channel can be terminated.
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.
Yeah, I was unsure if we need to use shutdownNow()
here to make sure it is quickly shut down. For cases like #7752, the reference to the current xDS channel may be quickly cleared and a new xDS channel is created. shutdownNow()
seems to be safer, doesn't it? Is there any reason that shutdown() is better?
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.
Well, as you said it doesn't actually matter much as long as streams are canceled when XdsClient shuts down. I am changing it to shutdown()
, we will see if any problem will be encountered.
1c0cf79
to
d80d7a0
Compare
This change fixes the problem of mismatched lifecycle of the xDS channel and XdsClient. Reading the bootstrap will determine and create the ChannelCredentials for each specified xDS server. An exception will be thrown if any xDS server specifies some channel_creds type that is not supported, not just for the first server (which is the only one to be used now). Reading the bootstrap also determines the xDS protocol version. The xDS channel will have the same lifecycle as the XdsClient instance: an xDS channel is created at the first call of getObject() and is shut down at the same time as the XdsClient is shutting down. A new xDS channel will be created when the ObjectPool creates a new XdsClient instance.
Fixes #7752.
The singleton XdsClient ObjectPool is cached for the entire process. The bootstrap file is read at the time this ObjectPool is created. The shared XdsClient instance has a much shorter lifecycle than the ObjectPool as the instance is only created upon the first call of
getObject()
and is destroyed when its reference count reaches zero. The ObjectPool creates a new XdsClient instance after the previous one was destroyed.The ownership and lifecycle of the xDS channel is wrong. Currently the singleton XdsClient ObjectPool owns the xDS channel and it uses the same xDS channel even when recreating a new XdsClient instance. However, the channel would have been shutdown by the XdsClient.
In this change:
ChannelCredentials
for each specified xDS server. An exception will be thrown if any xDS server specifies some channel_creds type that is not supported, not just for the first server (which is the only one to be used now).getObject()
and is shut down at the same time as the XdsClient is shutting down.Main changes are in
Bootstrapper.java
andSharedXdsClientPoolProvider.java
(andXdsClientWrapperForServerSds.java
for server side XdsClient creation)./cc @sanjaypujare