xds: change controlPlaneClient and loadReportClient to use xdsTransportFactory #10829
Conversation
| "Ignore an unknown type of DiscoveryResponse: {0}", | ||
| response.getTypeUrl()); | ||
|
|
||
| call.startRecvMessage(); |
There was a problem hiding this comment.
Why do the startRecvMessage here? If the flow goes down other paths it isn't done and wasn't done in the original.
There was a problem hiding this comment.
It is important to add startRecvMessage() here. Before this PR it is request(1).
This is for the flow control. We do it manually. For each response, we have to call request(1) at the end of the process. Normally it is at the end of handleRpcReponse in processTracker.
| call.startRecvMessage(); | ||
| return; | ||
| } | ||
| handleRpcResponse(type, response.getVersionInfo(), response.getResourcesList(), |
There was a problem hiding this comment.
Even though the underlying xdsResponseHandler.handleResourceResponse expects this format, it would be cleaner to pass handleRpcResponse the whole response message and let it do the parsing in case in the future it wants to consider something else from the response.
There was a problem hiding this comment.
Actually I think this is what the current structure is. This is the interface between xdsClientImpl and ControlPlaneClient. The response proto does not have too many interesting fields. Although it might be for sustainable if we define a Arg data structure instead of extending the parameter list. But to me it is fine for now and it is internal.
#10827