Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: add envoy-ratelimited retry policy #12201

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

numerodix
Copy link
Contributor

This PR implements a new retry policy that retries a request if the response contains the header x-envoy-ratelimited.

This is PR (1/2) that addresses issue #12200. See also the design document: https://docs.google.com/document/d/1NSzrx3-KsAFs0ObaLQZUVIt9O4PZv3mXi3Lm7LNzmZE/edit?usp=sharing

Signed-off-by: Martin Matusiak [email protected]

Risk Level: Low
Testing: New unit test added.
Docs Changes: added
Release Notes: added

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this looks good to me and is not controversial at all IMO, happy to add this given that there is now a need. Just one minor nit

@@ -290,9 +293,9 @@ RetryStatus RetryStateImpl::shouldHedgeRetryPerTryTimeout(DoRetryCallback callba
}

bool RetryStateImpl::wouldRetryFromHeaders(const Http::ResponseHeaderMap& response_headers) {
// We never retry if the request is rate limited.
// We retry our own rate limited requests only when the envoy-ratelimited policy is in effect.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Our own" implies to me that this envoy instance rate limited the request, but its likely going to be another Envoy instance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a good point. I'll rephrase this to make it clear we're talking about an upstream envoy.

Adds new retry policy envoy-ratelimited that retries responses
containing the header x-envoy-ratelimited.

Signed-off-by: Martin Matusiak <[email protected]>
Signed-off-by: Martin Matusiak <[email protected]>
Signed-off-by: Martin Matusiak <[email protected]>
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@snowp snowp merged commit 8b34cb1 into envoyproxy:master Jul 22, 2020
KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020
Adds new retry policy envoy-ratelimited that retries responses
containing the header x-envoy-ratelimited.

Signed-off-by: Martin Matusiak <[email protected]>
Signed-off-by: Kevin Baichoo <[email protected]>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
Adds new retry policy envoy-ratelimited that retries responses
containing the header x-envoy-ratelimited.

Signed-off-by: Martin Matusiak <[email protected]>
Signed-off-by: chaoqinli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants