Feature: http retry

[szuecs]

Feature: http retry in httpclient

fixes #1473

[szuecs]

Features that would be interesting
https://brooker.co.za/blog/2022/02/28/retries.html
https://aws.amazon.com/builders-library/timeouts-retries-and-backoff-with-jitter/

[szuecs]

this seems to work in all cases I try, but this only works for 3 different types of body creations https://github.com/golang/go/blob/master/src/net/http/request.go#L877-L881 as far as I understand.

[szuecs]

GetBody() has a problem: // Next line panics on TestClientRetryBodyHalfReader

[bjhaid]

From reading the code it is unclear if it tries a new backend. If it doesn't, is it desirable for the new retry filter to attempt a different backend from the previously failed one?

[szuecs]

@bjhaid yes in case of loadbalancer backend it would retry by "running" again the algorithm, so in case of roundRobin it would try the current "next". I guess it's fine to run the same algorithm again, because if you think about a single backend error. For example a broken node issue that makes a backend instance to a half broke state, that can respond to "/ready" but not handle most of the requests anymore. I would guess a "network" backend target would exactly do the same: It will run some lb algorithm in some load balancer in front of the application.
The retry executes https://github.com/zalando/skipper/pull/2333/files#diff-1d74870266949f1927a500e5f0002617239d3e3cd9bcceee6be014738e5e0f2bR1272 makeBackendRequest again, which contains the call to mapRequest() which will execute selectEndpoint which will execute the lb algorithm

Do you see an issue with this behavior?

[bjhaid]

The retry executes https://github.com/zalando/skipper/pull/2333/files#diff-1d74870266949f1927a500e5f0002617239d3e3cd9bcceee6be014738e5e0f2bR1272 makeBackendRequest again, which contains the call to mapRequest() which will execute selectEndpoint which will execute the lb algorithm

Do you see an issue with this behavior?

Thanks for the clarification! It wasn't obvious to me from reading the changeset. This behavior makes sense to me. I am also interested in seeing this ship thanks for reviving it ❤️

[AlexanderYastrebov]

Hm, I do not see a corresponding Delete. Looks like this will leak buffers.

[AlexanderYastrebov]

Do we need this in the client? Why not let users use CopyBodyStream like we do in proxy?

[szuecs]

The proxy code is about one request, but here it's about a client that could hundreds of requests and we need to get the right body for the retry. If we want to support a retry with body, we need to store it somewhere.

[szuecs]

I saw we can use LoadAndDelete() instead of Load(), but this would make only a single retry possible, but maybe that's good enough for now.

[szuecs]

Do we need this in the client? Why not let users use CopyBodyStream like we do in proxy?

I thought it makes sense to provide it for simplicity as a user.

[bjhaid]

@szuecs is there anything I can do to help make progress on this PR.... I will love to see this ship.

[szuecs]

@bjhaid sorry, I was in vacation and I just came back reading backlog of changes and emails and got a bit slurped into zone awareness. 😀
Let me see what is missing to make it happen.

A great way to help would be to tell what you expect from retry() and what kind of configuration you would like to pass to this filter.
Would you like to have a single retry or exponential backoff or maybe a token bucket style like Marc showed in https://brooker.co.za/blog/2022/02/28/retries.html ?

[bjhaid]

A great way to help would be to tell what you expect from retry() and what kind of configuration you would like to pass to this filter.

something like retry(int N) where N is the number of times it is retried(capped at some reasonable value) with a default like 3.