Include response chunking info in nonce #3
Conversation
If a given delta response contains too many resources, the server will break it up into multiple responses. However, this means the client does not know whether it received all the resources for its subscription. This is especially relevant for wildcard subscriptions, for which the client does not know the resources ahead of time and therefore cannot wait for them explicitly. By returning additional metadata in the nonce (there is no field for this in the delta discovery response, though I'm hoping that will change cncf/xds#99), the client can know if the server chunked the response, and react accordingly.
| // NewNonce creates a new unique nonce based on the current UNIX time in nanos, always returning a | ||
| // string of [NonceLength]. | ||
| func NewNonce(remainingChunks int) string { |
There was a problem hiding this comment.
a smart yet big change to the nonce. To make sure it's backward compatible, I checked Rest.li xds client is not using the nonce in any way currently. May want to let Xin and Yan know to make sure Envoy is not using it too.
There was a problem hiding this comment.
No one is, it's not supposed to be used for anything other than ACKs/NACKs, it's not meant to be interpreted more than that. Envoy doesn't use it at all for sure.
internal/server/handlers_delta.go
Outdated
| @@ -92,7 +92,7 @@ type deltaSender struct { | |||
| queuedUpdates []queuedResourceUpdate | |||
| // The minimum size an encoded chunk will serialize to, in bytes. Used to check whether a given | |||
| // update can _ever_ be sent, and as the initial size of a chunk. Note that this value only depends | |||
| // on utils.NonceLength and the length of typeURL. | |||
| // on utils.MaxNonceLength and the length of typeURL. | |||
ads/ads.go
Outdated
| // max gRPC message size of 4MB. A nonce from Diderot always starts with the 64-bit nanosecond | ||
| // timestamp of when the response was generated on the server. Then the number of remaining chunks as | ||
| // a 32-bit integer. The sequence of integers is binary encoded with [binary.BigEndian] then hex | ||
| // encoded. If the given nonce does not match the expected format, this function simply returns 0, as |
There was a problem hiding this comment.
Should this return -1 instead of 0 or some other special value if the nonce didn't match the expected format? It could also return remainingChunks, err if that would be more idiomatic
Otherwise, someone using ParseRemainingChunksFromNonce would be unable to distinguish "this is the last chunk in the response" from "the nonce didn't match the format so we don't know how many chunks remain in the response", right?
There was a problem hiding this comment.
That's kind of the point actually. By default, the behavior in xDS is to not wait for chunks, since that notion doesn't really exist. Ultimately, if the nonce doesn't have the right format, then the only valid behavior is not to wait, that's why it just returns 0. There's literally nothing a caller of this function could do.
There was a problem hiding this comment.
Zoab brought up a good point offline which is that returning an error at least signals to callers that the nonce wasn't in the right format, and even though they can't do anything about it, they can still log a warning.
If a given delta response contains too many resources, the server will break it up into multiple responses. However, this means the client does not know whether it received all the resources for its subscription. This is especially relevant for wildcard subscriptions, for which the client does not know the resources ahead of time and therefore cannot wait for them explicitly. By returning additional metadata in the nonce (there is no field for this in the delta discovery response, though I'm hoping that will change cncf/xds#99), the client can know if the server chunked the response, and react accordingly.