This repository was archived by the owner on Sep 26, 2023. It is now read-only.
feat: make stream wait timeout a first class citizen [WIP]#1409
Closed
igorbernstein2 wants to merge 4 commits intogoogleapis:masterfrom
Closed
feat: make stream wait timeout a first class citizen [WIP]#1409igorbernstein2 wants to merge 4 commits intogoogleapis:masterfrom
igorbernstein2 wants to merge 4 commits intogoogleapis:masterfrom
Conversation
For a server streaming api, there are 4 conceptual timeouts: 1. overall operation timeout - the maximum amount time that passes from the user invoking a method until that method exits 2. attempt/rpc timeout - if retries are enabled the maximum amount of time that passes for each attempt in an operation 3. message wait timeout - the maximum amount of time to wait for the next message from the server 4. idle timeout - how long to wait before considering the stream orphaned by the user and closing it Each has a usecase: 1. operation timeout is useful for users to fulfill their own slo guarantees 2. attempt timeout are useful when a client developer knows the absolute limit of an rpc but that limit happens to be shorter than the required slo for a customer. For example a point read of a bigtable key shouldnt ever take more than 100ms. If it does, then we can assume something is wrong (GFE died without sending a FIN packet) and abort the request and retry. 3. message wait timeouts have a similar use as attempt timeouts with tighter guarantees 4. idle timeouts are useful to reduce buffer bloat on the server Currently all of them are implemented in gax but the delineation was muddied by me a while back. This PR tries to fix the situation. In the current world, operation timeout is defined by RetrySettings#totalTimeout, idle timeout is defined by ServerStreamingCallSettings#idleTimeout. However RetrySettings#rpcTimeout is mapped to message wait timeout and attempt timeout is only configureable per call using ApiCallContext#withTimeout. This PR cleans up the situation by mapping RetrySettings#rpcTimeout to attempt timeouts and exposes a new setting for wait timeout on ServerStreamingCallSettings.
igorbernstein2
added
the
do not merge
google-cla
bot
added
the
cla: yes
igorbernstein2
changed the title
mutianf
reviewed
Jun 18, 2021
|
|
||
| if (totalTimeout != null && context != null && context.getTimeout() == null) { | ||
| context = context.withTimeout(totalTimeout); | ||
| } |
Contributor
There was a problem hiding this comment.
After removing this block, if someone set totalTimeout but not rpc timeout, the stream won't have a deadline?
Contributor
Author
|
I think there is too much risk with remapping rpcTimeout from response timeout to attempt timeout. I'm going to close this for now. If gax ever decided to do a major version bump, we can re-consider this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For a server streaming api, there are 4 conceptual timeouts:
Each has a usecase:
Currently all of them are implemented in gax but the delineation was muddied by me a while back. This PR tries to fix the situation. In the current world, operation timeout is defined by RetrySettings#totalTimeout, idle timeout is defined by ServerStreamingCallSettings#idleTimeout. However RetrySettings#rpcTimeout is mapped to message wait timeout and attempt timeout is only configureable per call using ApiCallContext#withTimeout.
This PR cleans up the situation by mapping RetrySettings#rpcTimeout to attempt timeouts and exposes a new setting for wait timeout on ServerStreamingCallSettings.