refactor retry state impl API#6132
Conversation
Previously there was a single shouldRetry() method which takes a HTTP header map and a stream reset reason and asserts that only one is set. In anticipation of adding a new retry case for envoyproxy#5841, this commit breaks apart this function into shouldRetryHeaders and shouldRetryReset. This leads to fewer asserts and more intentional code. Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks, this seems like an improvement
| DoRetryCallback callback) PURE; | ||
|
|
||
| /** | ||
| * Determine whether a request should be retried after a reset based on the reason for the rest. |
|
|
||
| RetryStatus RetryStateImpl::shouldRetryHeaders(const Http::HeaderMap* response_headers, | ||
| DoRetryCallback callback) { | ||
| ASSERT(response_headers != nullptr); |
There was a problem hiding this comment.
Instead of asserting that this is true, you can change response_headers to be passed by const ref which ensures that it's not null
|
|
||
| if (callback_ && !wouldRetry(response_headers, reset_reason)) { | ||
| RetryStatus RetryStateImpl::shouldRetry(RetryPredicate would_retry, DoRetryCallback callback) { | ||
| if (callback_ && !would_retry()) { |
There was a problem hiding this comment.
Not your code but could you add a comment explaining why this makes sense? Took me a minute
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks very nice, 1 question / small comment.
/wait
|
|
||
| RetryStatus retry_status = | ||
| retry_state_->shouldRetry(nullptr, reset_reason, [this]() -> void { doRetry(); }); | ||
| retry_state_->shouldRetryReset(reset_reason.value(), [this]() -> void { doRetry(); }); |
There was a problem hiding this comment.
How are we guaranteed that this option as a value()? It's not clear to me from the code flow. Can you add a comment or potentially look at changing the logic if always has a value?
Also, while you are here, can you change the call signature to pass the StreamResetReason by value and not reference? It will be faster (my own badly written code most likely).
There was a problem hiding this comment.
yeah I think I don't need an optional anymore, will do that
There was a problem hiding this comment.
nvm there is a place where it might not have a value (onResponseTimeout()). I actually take care of that case and remove the optional in #6142. For this PR i'll put a comment explaining why it will always have a value
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Previously there was a single shouldRetry() method which takes a HTTP header map and a stream reset reason and asserts that only one is set. In anticipation of adding a new retry case for envoyproxy#5841, this commit breaks apart this function into shouldRetryHeaders and shouldRetryReset. This leads to fewer asserts and more intentional code. Signed-off-by: Michael Puncel <mpuncel@squareup.com> Signed-off-by: Fred Douglas <fredlas@google.com>
3 participants
Previously there was a single shouldRetry() method which takes a HTTP
header map and a stream reset reason and asserts that only one is set.
In anticipation of adding a new retry case for #5841, this commit breaks
apart this function into shouldRetryHeaders and shouldRetryReset. This
leads to fewer asserts and more intentional code.
Signed-off-by: Michael Puncel mpuncel@squareup.com
Description: Refactor public API for RetryState.
Risk Level: Low
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A
This was done in anticipation of #5841 which will be adding a new retry condition (per try timeout) that does not result from a stream reset or response headers