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

retry filter #1473

Open
szuecs opened this issue Jul 7, 2020 · 7 comments · May be fixed by #2333
Open

retry filter #1473

szuecs opened this issue Jul 7, 2020 · 7 comments · May be fixed by #2333
Assignees
Labels
backlog maintainers backlog to use in planning enhancement

Comments

@szuecs
Copy link
Member

szuecs commented Jul 7, 2020

This looks great to use, maybe as filter https://github.com/sethvargo/go-retry
Needs to check what if we have non idempotent requests like POST requests, but in general as opt-in feature could be done.

@AlexanderYastrebov
Copy link
Member

Seems to be related to #674

@szuecs
Copy link
Member Author

szuecs commented Jan 6, 2023

@szuecs
Copy link
Member Author

szuecs commented Jan 27, 2023

Maybe http.Transport can already do this for us if we add idempotent header:

Transport only retries a request upon encountering a network error if the request is idempotent and either has no body or has its Request.GetBody defined. HTTP requests are considered idempotent if they have HTTP methods GET, HEAD, OPTIONS, or TRACE; or if their Header map contains an "Idempotency-Key" or "X-Idempotency-Key" entry. If the idempotency key value is a zero-length slice, the request is treated as idempotent but the header is not sent on the wire.

Cite is the last section of https://pkg.go.dev/net/http#Transport

@oporkka
Copy link
Member

oporkka commented Mar 6, 2024

We are running a Node.js application with very uneven complexity of requests, and I have been wondering if a retry-pattern from Skipper as ingress could help us to solve cases where one or few pods are overloaded and would fail fast, but still allow delivering a successful response to the clients.

For example

  1. Skipper requests pod "A"
  2. Pod "A" fails fast with 503
  3. Skipper tries one retry to another pod (next one in rotation)

@szuecs
Copy link
Member Author

szuecs commented Mar 6, 2024

@oporkka I expect that you need http body, right?

@oporkka
Copy link
Member

oporkka commented Mar 8, 2024

@oporkka I expect that you need http body, right?

Yes, I would need this for POST requests, which I know that would be idempotent failures (short-circuit). I wonder if this should be steered by a response header, in order to distinguish from other 503 cases which might not be idempotent.

@szuecs
Copy link
Member Author

szuecs commented Mar 11, 2024

@oporkka I think if we implement the decision as filter then I don't mind that it always retries also POST requests. I think in general if POST is to be retried or not should be done by creating the routes (ORing by Method("POST") or not).
If we check the response header then I think the affect will be that it's less interesting, because most incidents that I reviewed were a mix of 503 (for example read/write timeouts while streaming body in both directions and all kind of connect errors), 504 (proxy to backend timeout) first and short time later a flood of 499 (client time out).

@szuecs szuecs linked a pull request Mar 14, 2024 that will close this issue
@szuecs szuecs self-assigned this Apr 4, 2024
@szuecs szuecs added the backlog maintainers backlog to use in planning label Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog maintainers backlog to use in planning enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants