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

Extend config interface to support setting retryServerErrors #439

Merged
merged 2 commits into from Jun 24, 2022

Conversation

sebasslash
Copy link
Contributor

@sebasslash sebasslash commented Jun 21, 2022

Description

There were instances where users would experience a network failure during the metadata or /ping request when calling NewClient().

This PR adds a new field RetryServerErrors to the Config object, which sets the value for c.retryServerErrors during client initialization and has a default value of false. In short, the client exposes a receiver method RetryServerErrors(bool) which allows you to set the value of c.retryServerErrors which toggles the retry logic, however it requires an initialized client. This became a problem due to the network request made in NewClient() that wouldn't be retried should it fail -- there was no way for the user to enable the retry logic before calling NewClient()!

Making it an option in the client's configuration process will circumvent this issue.

tfe.go Outdated Show resolved Hide resolved
tfe.go Outdated Show resolved Hide resolved
@sebasslash sebasslash force-pushed the sebasslash/retry-ping-request branch from 3f564e7 to dfaa6f1 Compare Jun 21, 2022
Uk1288
Uk1288 previously approved these changes Jun 22, 2022
Copy link
Contributor

@Uk1288 Uk1288 left a comment

LGTM! 🎉

tfe.go Outdated Show resolved Hide resolved
The client exposes a receiver method called RetryServerErrors which toggles a flag, c.retryServerErrors,
that enables/disables the retry logic in the http client. This means however, you could only enable
the retry logic once the client was created, meaning any failed metadata request in `NewClient()`
would not be retried. We've extended the interface of the client config object to allow
enabling `c.retryServerErrors` during client init `NewClient()`.
Copy link
Collaborator

@brandonc brandonc left a comment

I don't think this does what it says in the description?

@sebasslash sebasslash changed the title Temporarily enable retryServerErrors before metadata request Extend config interface to support setting retryServerErrors Jun 23, 2022
annawinkler
annawinkler previously approved these changes Jun 23, 2022
Copy link
Contributor

@annawinkler annawinkler left a comment

We reviewed this PR yesterday. Looks good! Please update the description accordingly. 🌟

@sebasslash
Copy link
Contributor Author

sebasslash commented Jun 23, 2022

I don't think this does what it says in the description?

Updated! 👍

brandonc
brandonc previously approved these changes Jun 23, 2022
@sebasslash sebasslash dismissed stale reviews from brandonc and annawinkler via 8bf7b8e Jun 23, 2022
@@ -66,17 +66,21 @@ type Config struct {

// RetryLogHook is invoked each time a request is retried.
RetryLogHook RetryLogHook

// RetryServerErrors enables the retry logic in the client.
Copy link
Collaborator

@brandonc brandonc Jun 23, 2022

Choose a reason for hiding this comment

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

I think we should be specific about which errors are retried here and in the CHANGELOG. I think it's just 500 and 429?

@sebasslash sebasslash merged commit cc5d816 into main Jun 24, 2022
6 checks passed
@sebasslash sebasslash deleted the sebasslash/retry-ping-request branch Jun 24, 2022
@github-actions
Copy link

github-actions bot commented Jun 24, 2022

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

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

5 participants