Conversation
mislav
left a comment
There was a problem hiding this comment.
Thank you! This looks like a good start, but I'm concerned about having to change existing methods in order to shoehorn the retry mechanism in. Couldn't it be done as a transparent feature of the API client?
|
|
||
| a.setHeaders(req) | ||
| resp, err := a.do(ctx, req, "/user/codespaces/*") | ||
| resp, err := a.withRetry(func() (*http.Response, error) { |
There was a problem hiding this comment.
Couldn't we avoid making any changes to methods like GetCodespace or StartCodespace and implement retries transparently within a.do()? That way the retry mechanism could just kick in for any request that we consider idempotent (typically, GET requests or POST to the start endpoint).
There was a problem hiding this comment.
@mislav the problem with a.do() is two fold:
- Makes it harder to control which methods we want to retry.
- More importantly, if the server ever returns redirects,
a.do()does not recreate the request therefore net/http will not be able to send the request body again in the case of POST requests. FYI: the net/http library does resend the body in the case of*bytes.Bufferand*bytes/strings.Reader, but that cannot be guaranteed that a caller won't use another underlyingio.Readerwhere the server sends a redirect response.
There was a problem hiding this comment.
Great point about the difficulties of retrying requests that have a body.
This comment was marked as spam.
This comment was marked as spam.
|
|
||
| a.setHeaders(req) | ||
| resp, err := a.do(ctx, req, "/user/codespaces/*") | ||
| resp, err := a.withRetry(func() (*http.Response, error) { |
There was a problem hiding this comment.
Great point about the difficulties of retrying requests that have a body.
This PR adds custom retries to a couple of our Codespaces API calls that may return a 502. Since we only need two calls to be retried for now, I did not introduce a new library that implements retries on the
*http.Clientbut instead just write a small 10 line function.If more and more API calls need to be retried, then we can certainly add a more generic solution.
Closes internal issue codespaces#5474