xds: refactor XdsClient in preparation to support federation #8630
Conversation
| parsedResources.put(routeConfigName, new ParsedResource(rdsUpdate, resource)); | ||
| } | ||
| getLogger().log(XdsLogLevel.INFO, | ||
| logger.log(XdsLogLevel.INFO, |
There was a problem hiding this comment.
log the serverInfo target as well? same all other places.
There was a problem hiding this comment.
Good point. I add serverInfo info in the logger of AbstractXdsClient.
| restartTimer(); | ||
| } | ||
|
|
||
| // TODO(zdapeng): add resourceName arg and support xdstp:// resources |
There was a problem hiding this comment.
For LDS, we will need to parse resourceName and lookup authority in bootstrapInfo to decide the serverInfo, right?
For other xds resources, is your current implementation sufficient to decide which serverInfo to use?
There was a problem hiding this comment.
The watcher will parse the resource name and get the authority
https://github.com/grpc/proposal/blob/9d1e77ff3103c5dac71eabcda4269f52764518b3/A47-xds-federation.md#watcher-changes
If the resource is not cached, it will getOrCreate a XdsChannel from the serverChannelMap (I'm not 100% sure how it will work for the case of xds server fail over in the future) and send a request.
There was a problem hiding this comment.
Yea the watcher change look up and lazy creating the channel makes sense.
You obviously discarded the two level map for resource cache implementation suggestion. The authority is actually included in resource name, but it is not the way gRFC suggested.
It is not a problem for now, not sure if it is a problem for server failover, or each authority can specify a list of servers, ordered by priority feature. Not a big concern for me for now.
|
|
||
| @Override | ||
| protected void handleShutdown() { | ||
| void shutdown() { |
There was a problem hiding this comment.
not running in sync context? previously it was. It might race with creating AbstractXdsClient.
It looks isShutdown() is not checked anywhere?
There was a problem hiding this comment.
Good catch. Should run in sync context. Yeah isShutdown() is only used for test.
See go/java-xds-client-api-for-federation for detailed description
See go/java-xds-client-api-for-federation for detailed description