Refactor onUpstreamReset in router filter.#6142
Refactor onUpstreamReset in router filter.#6142mattklein123 merged 11 commits intoenvoyproxy:masterfrom
Conversation
source/common/router/router.cc
Outdated
There was a problem hiding this comment.
i'm basically redefining what "upstream reset" means here. it used to mean a reset by the upstream, or a local reset of the upstream request (timeout). Now it only means the upstream reset it because we handle global/per try timeouts in other methods
snowp
left a comment
There was a problem hiding this comment.
Thanks, I think this refactor makes sense
Previously onUpstreamReset handled 3 separate cases: per try timeout, global timeout, and a stream reset by the upstream. In anticipation of adding a new case for envoyproxy#5841, it might make things cleaner to pull it out. This commit extracts the functionality of the old onUpstreamReset into separate methods, and then has onPerTryTimeout, onGlobalTimeout, and onUpstreamReset call the helper functions with the the appropriate arguments. Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
6269ed5 to
d0a8f4e
Compare
|
rebased to resolve conflicts and pushed a new commit addressing PR feedback |
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
41da678 to
8613eb3
Compare
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this is a super awesome cleanup. Very nice to see this code getting some attention! Few small comments.
/wait
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
|
@mpuncel I think the issue is the trailing envoy/source/common/router/router.h Line 44 in 14c4131 I've seen this before, I think it prevents clang-format from turning back on. Can you remove that and try formatting again? |
…mat fix Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Previously onUpstreamReset handled 3 separate cases: per try timeout,
global timeout, and a stream reset by the upstream. In anticipation of
adding a new case for #5841, it might make things cleaner to pull it
out.
This commit extracts the functionality of the old onUpstreamReset into
separate methods, and then has onPerTryTimeout, onGlobalTimeout, and
onUpstreamReset call the helper functions with the the appropriate
arguments.
Signed-off-by: Michael Puncel mpuncel@squareup.com
Description: Refactor onUpstreamReset in the router filter from handling multiple cases (per try timeout, global timeout, upstream reset) and decompose it into helper functions called from case-specific handlers.
Risk Level: medium
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A
This is a step toward #5841 which will have different handling for "soft" per try timeouts and normal per try timeout, which would make onUpstreamReset more complex than it already is.