router: allowing request-header-based retry checks#8449
router: allowing request-header-based retry checks#8449mattklein123 merged 4 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| // HTTP response headers that trigger a retry if present in the response. A retry will be | ||
| // triggered if any of the header matches match the upstream response headers. | ||
| // The field is only consulted if 'retriable-headers' retry policy is active. | ||
| repeated HeaderMatcher retriable_headers = 9; |
There was a problem hiding this comment.
in retrospect this probably should have been called retriable_response_headers :)
There was a problem hiding this comment.
@htuch shall we handle field rename like this in protoxform when upgrade to next-major-version?
There was a problem hiding this comment.
Yeah we should add this capability as an annotation or comment.
There was a problem hiding this comment.
Yes, I will be adding annotations for this later. For now, you can use [#next-major-version:] or file an explicit GitHub issue and put it in the v3 API label.
source/common/router/config_impl.cc
Outdated
| ProtobufMessage::ValidationVisitor& validation_visitor) | ||
| : retriable_headers_( | ||
| Http::HeaderUtility::buildHeaderMatcherVector(retry_policy.retriable_headers())), | ||
| retriable_request_headers_(optionalHeaderMatcher(retry_policy.retriable_request_headers())), |
There was a problem hiding this comment.
should retriable_headers_ also be optional and use the optionalHeaderMatcher helper? is there a reason for these two to be different?
source/common/router/config_impl.h
Outdated
| uint32_t host_selection_attempts_{1}; | ||
| std::vector<uint32_t> retriable_status_codes_; | ||
| std::vector<Http::HeaderMatcherSharedPtr> retriable_headers_; | ||
| absl::optional<std::vector<Http::HeaderMatcherSharedPtr>> retriable_request_headers_; |
There was a problem hiding this comment.
if having it be optional a memory optimization? iirc making it optional doesn't decrease its size: https://github.com/abseil/abseil-cpp/blob/master/absl/types/optional.h#L91
There was a problem hiding this comment.
Yeah, I assumed naively that it saved memory. TIL.
I do worry about the memory impact of feature creep on the retry state, but given it's include not apis we can always switch to pointers (which definitely don't take as much space) at some point in the future, so I'll stick with the types we're already using for now.
| Http::TestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; | ||
| setup(request_headers); | ||
| EXPECT_FALSE(state_->enabled()); | ||
| EXPECT_EQ(RetryStatus::No, state_->shouldRetryReset(overflow_reset_, callback_)); |
There was a problem hiding this comment.
probably not needed to check this?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| // HTTP response headers that trigger a retry if present in the response. A retry will be | ||
| // triggered if any of the header matches match the upstream response headers. | ||
| // The field is only consulted if 'retriable-headers' retry policy is active. | ||
| repeated HeaderMatcher retriable_headers = 9; |
There was a problem hiding this comment.
Yeah we should add this capability as an annotation or comment.
|
|
||
| const std::vector<uint32_t> retriable_status_codes_{}; | ||
| const std::vector<Http::HeaderMatcherSharedPtr> retriable_headers_{}; | ||
| const std::vector<Http::HeaderMatcherSharedPtr> retriable_request_headers_{}; |
There was a problem hiding this comment.
super nit: The {} is not needed on any of these.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Risk Level: Low (new code is config guarded)
Testing: new unit tests
Docs Changes: n/a
Release Notes: yes