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

Switch to HTTP retry library #26

Merged
merged 2 commits into from Aug 16, 2022
Merged

Switch to HTTP retry library #26

merged 2 commits into from Aug 16, 2022

Conversation

thomshutt
Copy link
Contributor

We were getting errors because of not resetting the POST body correctly before retrying, so decided to switch to a library and not have to think about that stuff.

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

Nice! Let's make catalyst-api the testground for this lib and consider applying it in most of our services (especially go-livepeer which handles large payloads all around)

type CallbackClient struct {
httpClient *http.Client
httpClient *retryablehttp.Client
Copy link
Member

Choose a reason for hiding this comment

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

WDYT of keeping this as a regular http.Client, just so we can switch from the retry lib more seamlessly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http.Client isn't an interface though, how would we switch between the two?

Copy link
Member

Choose a reason for hiding this comment

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

There's apparently a function in the retry client to return a corresponding http.Client! I saw it mentioned in the end of their readme I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh nice, I had seen that but had missed that it includes the retry functionality

StandardClient returns a stdlib *http.Client with a custom Transport, which shims in a *retryablehttp.Client for added retries.

Will make a new PR for that

Comment on lines 32 to 43
func (c CallbackClient) DoWithRetries(r *retryablehttp.Request) error {
resp, err := c.httpClient.Do(r)
if err != nil {
return fmt.Errorf("failed to send callback to %q. Error: %s", r.URL.String(), err)
}

if err != nil {
return fmt.Errorf("failed to send callback to %q. Error: %q", r.URL.String(), err)
if resp.StatusCode >= 400 {
return fmt.Errorf("failed to send callback to %q. HTTP Code: %d", r.URL.String(), resp.StatusCode)
}
return fmt.Errorf("failed to send callback to %q. Response Status Code: %d", r.URL.String(), resp.StatusCode)

return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this still necessary? I think it could be replaced by a simpler helper like responseToError(res *http.Response) error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I 100% understand where the helper would sit, but we do still need the StatusCode check since the retry library only errors on >= 500

Co-authored-by: Victor Elias <victorgelias@gmail.com>
@thomshutt thomshutt merged commit 4d1416e into main Aug 16, 2022
@thomshutt thomshutt deleted the retry-lib branch August 16, 2022 10:12
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.

None yet

2 participants