[ADR] RPC streaming design doc #952
Conversation
This reverts commit 66556a5.
…tions-sdks into timtay/streaming
|
We should consider the scenario where the executor restarts while it has only received half of the request stream, and call out the executor handling in that case explicitly. |
| ```0:true:true:0```: This request stream has been canceled. Note that the values for ```index```, ```isLast```, and ```<rpc timeout milliseconds>``` are ignored here. | ||
|
|
||
| ```0:true:true```: This response stream has been canceled. Note that the values for ```index``` and ```isLast``` are ignored here. |
There was a problem hiding this comment.
why wouldn't index be relevant for these cases?
There was a problem hiding this comment.
In so far as they won't be presented to the user as though they were messages in the stream. Our protocol layer would take these messages and turn them into signaling a cancellation token to let the user's application layer know to stop processing this stream.
| /// If the stream has not been cancelled, this will return null. If the stream has been cancelled, but no user properties were | ||
| /// provided in that cancellation request, this will return null. | ||
| /// </remarks> | ||
| Dictionary<string, string>? GetCancellationRequestUserProperties(); |
There was a problem hiding this comment.
why not have these as some information provided with the cancellation? (Or is this just a dotnet limitation)
There was a problem hiding this comment.
This is kind of a .NET limitation. For all things cancellation, we use the cancellation token class. But the cancellation token class has no user properties.
There was a problem hiding this comment.
I am curious how you would approach this in Rust though because I'm not in love with my current .NET approach
There was a problem hiding this comment.
I don't have something exact, but I imagine we'd do some sort of wrapper around a standard cancellation token to expose these fns. Here's a very rough psuedo code that matches what you have here in dotnet (although I skipped the timeout for nowl)
pub struct RPCStream {
...
}
impl RPCStream {
pub async fn recv_next_entry() -> Option<T>;
pub async fn cancel(user_properties) -> Result;
// not sure if this would be needed in rust, but left it for now in case
pub fn is_cancelled() -> bool;
// this is the rust cancellation pattern that allows you to `await` until this is cancelled
pub async fn cancelled() -> UserProperties;
}
| - The same correlation data as the invoked method | ||
| - Streaming metadata with the ["cancel" flag set](#streaming-user-property) | ||
| - No payload | ||
| - The command invoker should still listen on the response topic for a response from the executor which may still contain a successful response (if cancellation was received after the command completed successfully) or a response signalling that cancellation succeeded ("Canceled" error code) |
There was a problem hiding this comment.
It seems like there are a few cancellation acknowledgement flows here - should some of this be removed, or are there distinct situations that I'm missing?
Invoker sends cancellation with cancel flag set -> executor responds with error code cancelled
Executor sends cancellation with cancel flag set -> invoker response with error code cancelled
Executor sends cancellation with cancel flag set -> invoker response with "stream cancelled successfully" flag set
If something should be removed, my preference would be for symmetry between the two directions (either they both respond with the error code or both respond with the success flag. Slight preference against the error code since it doesn't feel like an error)
There was a problem hiding this comment.
I may not have made this clear enough, but there is no "success" response to a cancellation request.
The receiving end of a cancellation request (invoker or executor) can either:
- Send the "canceled" error code
- Send nothing if it already sent the final message in its sending stream and already received the final message in its receiving stream ("You can't cancel this because it is already done")
There was a problem hiding this comment.
Does that explanation make more sense? I can update the doc if so
| - Allow for an arbitrary number of command requests and responses for a single command invocation | ||
| - The total number of requests and responses does not need to be known before the first request/response is sent | ||
| - The total number of entries in a stream is allowed to be 1 | ||
| - When exposed to the user, each request and response includes an index of where it was in the stream |
There was a problem hiding this comment.
Aren't they already ordered by __ts and __ft ?
| /// <param name="streamExchangeTimeout">The timeout between the beginning of the request stream and the end of both the request and response stream.</param> | ||
| /// <param name="cancellationToken">Cancellation token. Signalling this will also make a single attempt to notify the executor of the cancellation.</param> | ||
| /// <returns>The stream of responses.</returns> | ||
| public async Task<IStreamContext<StreamingExtendedResponse<TResp?>> InvokeStreamingCommandAsync( |
There was a problem hiding this comment.
Can we please use IAsyncEnumerable<StreamingExtendedResponse<TResp?>> as return value?
| - API design is messy because a command invoker/executor should not expose streaming command APIs if they have no streaming commands | ||
| - Caching behavior of normal RPC doesn't fit well with streamed RPCs which may grow indefinitely large | ||
|
|
||
|
|
There was a problem hiding this comment.
In case of protocol error or exception in executor, will be __stat not equal to 200 automatically seen as last message?
| - Caching behavior of normal RPC doesn't fit well with streamed RPCs which may grow indefinitely large | ||
|
|
||
|
|
||
| ## Appendix |
There was a problem hiding this comment.
Can each streaming message contain application level errors?
No description provided.