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

new: retry on common errors #505

Merged
merged 14 commits into from
Aug 14, 2023
Merged

Conversation

jriddle-linode
Copy link
Collaborator

@jriddle-linode jriddle-linode commented Aug 4, 2023

📝 Description

What does this PR do and why is this change necessary?

Adding retry functionality for common random errors sent by the API

  • 400: Bad Request with nginx headers
  • 408: Request Timeout
  • 429: Rate limit exceeded

✔️ How to Test

What are the steps to reproduce the issue or verify the changes?

How do I run the relevant unit/integration tests?

make test

@jriddle-linode jriddle-linode requested a review from a team as a code owner August 4, 2023 17:28
Copy link
Contributor

@ykim-akamai ykim-akamai left a comment

Choose a reason for hiding this comment

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

Looks good!

@jriddle-linode jriddle-linode requested a review from a team August 9, 2023 18:30
@zliang-akamai
Copy link
Member

zliang-akamai commented Aug 9, 2023

In Python SDK we used the retry class from urllib3. Should we consider that for linode-cli as well or?

https://github.com/linode/linode_api4-python/blob/dev/linode_api4/linode_client.py#L26

@lgarber-akamai
Copy link
Contributor

lgarber-akamai commented Aug 14, 2023

In Python SDK we used the retry class from urllib3. Should we consider that for linode-cli as well or?

https://github.com/linode/linode_api4-python/blob/dev/linode_api4/linode_client.py#L26

@zliang-akamai That's a great question! The biggest limitation I can think of with that approach is that it might be a good bit more complicated to implement complex retry logic (e.g. 400 NGINX errors).

I'm also not entirely sure whether urllib3 respects the Retry-After header. edit: Nevermind, looks like it does.

Copy link
Contributor

@lgarber-akamai lgarber-akamai left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just needs a make format run .

@jriddle-linode jriddle-linode merged commit cfdc3d4 into linode:dev Aug 14, 2023
@jriddle-linode jriddle-linode deleted the new/retry branch August 14, 2023 20:45
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.

5 participants