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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure retry mechanism #108

Merged
merged 7 commits into from
Apr 8, 2024
Merged

Restructure retry mechanism #108

merged 7 commits into from
Apr 8, 2024

Conversation

ziscky
Copy link
Contributor

@ziscky ziscky commented Mar 28, 2024

ClickUp: https://app.clickup.com/t/86942wrhv

  • Wrap the same backoff library used by resty
  • Create a method to retry any function ( for use in cases where we can't replace the http client )

馃敆 zboto Link

Copy link
Contributor

@lucaslopezf lucaslopezf left a comment

Choose a reason for hiding this comment

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

By setting the backoff this way, we disrupt the httpClient interface as well as the mocks used for testing. We could consider adopting a backoff strategy similar to the one used in Resty (following the builder pattern): https://github.com/go-resty/resty?tab=readme-ov-file#retries. Doing so would align our HTTP client with a standard, making it more intuitive and testable

@ziscky ziscky requested a review from lucaslopezf March 28, 2024 12:04
Copy link
Member

@emmanuelm41 emmanuelm41 left a comment

Choose a reason for hiding this comment

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

I like the approach. Let's wait for Lucas approval, but it is ok for me!

Copy link
Contributor

@lucaslopezf lucaslopezf left a comment

Choose a reason for hiding this comment

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

lgtm! 馃挭

@emmanuelm41 emmanuelm41 merged commit ecca9f3 into main Apr 8, 2024
4 checks passed
@emmanuelm41 emmanuelm41 deleted the feat/generic-backoff branch April 8, 2024 11:53
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

3 participants