grpc-js: Add XdsClient class #1489
Conversation
murgatroid99
force-pushed
the
grpc-js_xds_client
branch
from
095a3b4
to
5767f7d
Compare
|
This is pretty much ready for review, but I want to keep it from being merged while it has a dependency on a prerelease version of |
This reverts commit 898ba43.
There was a problem hiding this comment.
Overall I think I am able to follow what the XdsClient class does (The xds-bootstrap.ts basically only exports the loadBootstrapInfo() promise to be used by the XdsClient.) The central piece of the implementation is this this.adsCall bidi-stream, and this code around it is to fulfill the xds spec. For example, ok I received a DiscoveryResponse__Output but it's an unknown type_url, I will write it back to this.adsCall as an error to notify the other side of the connection. And there are all sorts of other reasons to write back on the this.adsCall stream.
The central use case is after the public method addEndpointWatcher() is called, we write out the entire watch list to the this.adsCall stream again, and the parse / validate the DiscoveryResponse__Output.
Now at least it all makes sense to me. Sorry to not being able to offer anything deeper. I have just a couple of minor JS questions (one about using this in callback, and another about comparing 0 with ===).
| if (this.hasShutdown) { | ||
| return; | ||
| } | ||
| this.adsCall = this.client.StreamAggregatedResources(); |
There was a problem hiding this comment.
What does this StreamAggregatedResources() do? I can't seem to grep this in the immediate surroundings.
Oh nevermind, this.client is a envoy.service.discovery.v2.AggregatedDiscoveryService, so I suppose StreamAggregatedResources() is a function provided there, which should have created an AdsCall.
| }); | ||
| const endpointWatcherNames = Array.from(this.endpointWatchers.keys()); | ||
| if (endpointWatcherNames.length > 0) { | ||
| this.adsCall.write({ |
There was a problem hiding this comment.
Are there additional trigger points to using this this.adsCall.write() bidi call other than from either 1. from the constructor, and 2. from the ServiceError just above? I was trying to think what situation will you be writing more than once?
So let's say, in a normal use case
constuctor -> maybeStartAdsStream() -> you create this.adsCall -> you call this.adsCall.write() -> you expect data back via the this.adsCall.on('data') and this.adsCall.on('error') callback just above -> now let's say there's a ServiceError above -> this.adsCall = null -> maybeStartAdsStream() again -> but that would have been a separate bidi call at that point.
What trigger point would have called this this.adsCall.write() multiple times? Or would a server-streaming call suffice?
[Edit:] Nevermind, I later realized that the stream is used by other functions like updateEdsNames, ackEds, nackEds, etc for all sort of reasons. So a bidi-stream is needed.
There was a problem hiding this comment.
One thing to note here is that AggregatedDiscoveryService is a service definition provided by Envoy that we are using. We don't decide anything about how the service is defined.
Also, the general StreamAggregatedServices workflow looks like this:
-
The client a request message with a list of names of resources of a specific type. Later, we can send a new message with a different list of names to change what resources we are interested in.
-
The server sends response message with a list of resources of that type. The server sends a new message whenever a resource is added, removed, or changed.
-
For each response message, the client either acknowledges (
ack) or rejects (nack) the update.
Within a single stream, each of these things happens independently for each type URL.
Hopefully this makes it clearer why we use a bidi stream for this method.
| trace( | ||
| 'ADS stream ended. code=' + error.code + ' details= ' + error.details | ||
| ); | ||
| this.adsCall = null; |
There was a problem hiding this comment.
Is it safe to use this inside the callback?
There was a problem hiding this comment.
In short, yes.
One of the benefits of using arrow functions ((args...) => {/*...*/}) is that they are bound to the scope where they are declared, not where they are called. The this reference is valid in the maybeStartAdsStream method where this callback is declared, so it is valid here too.
| // Also do the same for other types of watchers when those are implemented | ||
| } | ||
|
|
||
| addEndpointWatcher( |
There was a problem hiding this comment.
I am slightly confused about this. So let's say I construct a XdsClient class instance, which immediately started the this.adsCall bidi stream. L236 is an empty list for now. Later I can call this method to add eds endpoint to watch. How do we get back to L236 so that we write out the eds endpoint we want to watch?
[Edit:] this.updateEdsNames() is the trigger to write out to the this.adsCall bidi-stream.
There was a problem hiding this comment.
Yes, updateEdsNames tells the server the new list of EDS names that we are interested in. It should be called whenever a name is added or removed from the list.
|
|
||
| private updateEdsNames() { | ||
| if (this.adsCall) { | ||
| this.adsCall.write({ |
There was a problem hiding this comment.
Oh, I guessed I answered my owned questions. Ha. So each time you call addEndpointWatcher, it triggers a this.adsCall.write() on an updated list of this.endpointWatchers names/keys. Ah, that also explains my previous question of why bidi stream is needed.
| if (entryIndex >= 0) { | ||
| watchersEntry.splice(entryIndex, 1); | ||
| } | ||
| if (watchersEntry.length === 0) { |
There was a problem hiding this comment.
Does comparing with 0 have to be ===? I guess it doesn't hurt.
There was a problem hiding this comment.
The convention is that all comparisons should be done using ===.
This class will be used for all xDS gRPC operations. This PR just implements the EDS operations. The other operations will be implemented along with their corresponding load balancer implementations.
The interesting files here are
xds-client.tsandxds-bootstrap.ts. The rest are submodules, generated code, and dependency updates.