Feature: http retry #2333
Feature: http retry #2333
Conversation
|
Features that would be interesting |
szuecs
force-pushed
the
feature/http-retry
branch
from
6b74ba9 to
5608ee7
Compare
net/httpclient.go
Outdated
| return c.Do(req) | ||
| } | ||
|
|
||
| if rc, err := req.GetBody(); err == nil { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
GetBody() has a problem: // Next line panics on TestClientRetryBodyHalfReader
szuecs
added
the
major
szuecs
force-pushed
the
feature/http-retry
branch
from
a834d90 to
ea54f7e
Compare
|
From reading the code it is unclear if it tries a new backend. If it doesn't, is it desirable for the new |
|
@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. 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 ❤️ |
| @@ -125,9 +132,41 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { | |||
| req.Header.Set("Authorization", "Bearer "+string(b)) | |||
| } | |||
| } | |||
| if req.Body != nil && req.Body != http.NoBody && req.ContentLength > 0 { | |||
| retryBuffer := skpio.NewCopyBodyStream(int(req.ContentLength), &bytes.Buffer{}, req.Body) | |||
| c.retryBuffers.Store(req, retryBuffer) | |||
There was a problem hiding this comment.
Hm, I do not see a corresponding Delete. Looks like this will leak buffers.
There was a problem hiding this comment.
Do we need this in the client? Why not let users use CopyBodyStream like we do in proxy?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
szuecs
force-pushed
the
feature/http-retry
branch
3 times, most recently
from
96e1c7d to
5aa5388
Compare
|
@szuecs is there anything I can do to help make progress on this PR.... I will love to see this ship. |
|
@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. 😀 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 |
szuecs
force-pushed
the
feature/http-retry
branch
from
5aa5388 to
46a8ce4
Compare
feature: retry() filter Signed-off-by: Sandor Szücs <[email protected]>
Signed-off-by: Sandor Szücs <[email protected]>
szuecs
force-pushed
the
feature/http-retry
branch
from
46a8ce4 to
0a27cce
Compare
Feature: http retry in httpclient
fixes #1473