router: defer per try timeout until downstream request is done #6643
Conversation
This file contains 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
This defers starting the per try timeout timer until onRequestComplete to ensure that it is not started before the global timeout. This ensures that the per try timeout will not take into account the time spent reading the downstream, which should be responsibility of the HCM level timeouts. Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
mpuncel
reviewed
Apr 18, 2019
source/common/router/router.h
Outdated
| @@ -434,6 +435,9 @@ class Filter : Logger::Loggable<Logger::Id::router>, | |||
| bool is_retry_ : 1; | |||
| bool include_attempt_count_ : 1; | |||
| bool attempting_internal_redirect_with_complete_stream_ : 1; | |||
| // Tracks whether we deferred a per try timeout because the downstream request | |||
| // had not been completed yet. | |||
| bool pending_per_try_timeout_ : 1; | |||
There was a problem hiding this comment.
let's put this on the UpstreamRequest instead of on the parent, that will hold up better once we have multiple upstream requests
mpuncel
reviewed
Apr 18, 2019
source/common/router/router.cc
Outdated
| @@ -521,6 +521,10 @@ void Filter::onRequestComplete() { | |||
| response_timeout_ = dispatcher.createTimer([this]() -> void { onResponseTimeout(); }); | |||
| response_timeout_->enableTimer(timeout_.global_timeout_); | |||
| } | |||
|
|
|||
| if (pending_per_try_timeout_) { | |||
There was a problem hiding this comment.
this might be better as a for loop over upstream requests to check their pending_per_try_timeout_ flag
Signed-off-by: Snow Pettersen <snowp@squareup.com>
mpuncel
previously approved these changes
Apr 18, 2019
|
/retest |
|
🔨 rebuilding |
mattklein123
requested changes
Apr 19, 2019
source/common/router/router.h
Outdated
| @@ -355,6 +355,9 @@ class Filter : Logger::Loggable<Logger::Id::router>, | |||
| bool upstream_canary_ : 1; | |||
| bool encode_complete_ : 1; | |||
| bool encode_trailers_ : 1; | |||
| // Tracks whether we deferred a per try timeout because the downstream request | |||
| // had not been completed yet. | |||
| bool pending_per_try_timeout_ : 1; | |||
There was a problem hiding this comment.
nit: I would probably name this something like create_per_try_timeout_on_request_complete_ or something like that to differentiate between waiting for the per try timeout to elapse which is what this sounds like to me.
Signed-off-by: Snow Pettersen <snowp@squareup.com>
mattklein123
requested changes
Apr 20, 2019
docs/root/intro/version_history.rst
Outdated
| @@ -11,6 +11,9 @@ Version history | |||
| * redis: added :ref:`prefix routing <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.prefix_routes>` to enable routing commands based on their key's prefix to different upstream. | |||
| * redis: add support for zpopmax and zpopmin commands. | |||
| * router: added ability to control retry back-off intervals via :ref:`retry policy <envoy_api_msg_route.RetryPolicy.RetryBackOff>`. | |||
| * router: per try timeouts will no longer start before the downstream request has been received | |||
| in full by the router. This ensure that the per try timeout does not account for slow | |||
docs/root/intro/version_history.rst
Outdated
| @@ -11,6 +11,9 @@ Version history | |||
| * redis: added :ref:`prefix routing <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.prefix_routes>` to enable routing commands based on their key's prefix to different upstream. | |||
| * redis: add support for zpopmax and zpopmin commands. | |||
| * router: added ability to control retry back-off intervals via :ref:`retry policy <envoy_api_msg_route.RetryPolicy.RetryBackOff>`. | |||
| * router: per try timeouts will no longer start before the downstream request has been received | |||
| in full by the router. This ensure that the per try timeout does not account for slow | |||
| downstreams and that will not not start before the global timeout. | |||
|
/retest |
|
🔨 rebuilding |
mpuncel
added a commit
to mpuncel/envoy
that referenced
this pull request
Apr 22, 2019
* master: thread: remove ThreadFactorySingleton (envoyproxy#6658) router: support offseting downstream provided grpc timeout (envoyproxy#6628) router: defer per try timeout until downstream request is done (envoyproxy#6643) update bazel readme for clang-format-8 on mac (envoyproxy#6660) Implement some TODOs in quic_endian_impl.h (envoyproxy#6644) docs: add aspell to mac dependencies to fix check format script (envoyproxy#6661) config: fix delta xDS's use of (un)subscribe fields, more explicit protocol spec (envoyproxy#6545) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This defers starting the per try timeout timer until onRequestComplete
to ensure that it is not started before the global timeout. This ensures
that the per try timeout will not take into account the time spent
reading the downstream, which should be responsibility of the HCM level
timeouts.
Signed-off-by: Snow Pettersen snowp@squareup.com
Risk Level: Medium, changes to router logic
Testing: Added new UT, updated existing ones
Docs Changes: n/a
Release Notes: n/a
Fixes #6624