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

replace httr::GET() with httr::RETRY() #301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ataustin
Copy link

Thanks for for this awesome package! I'm working on chircollab/chircollab20#1 as part of Chicago R Collab.

In this PR, I'd like to propose swapping out calls to httr::GET() etc. with httr::RETRY(). This will make the package more resilient to transient problems like brief network outages or periods where the service(s) it hits are overwhelmed. In my experience, using retry logic almost always improves the user experience with HTTP clients.

Thanks for your consideration!

Before you open your PR

  • Did you run R CMD CHECK?
  • Did you run roxygen2::roxygenise(".")?

Thanks for contributing!

@dkahle
Copy link
Owner

dkahle commented Apr 20, 2020

Cool. A few thoughts.

  1. Would you mind updating the NEWS.md to alert users?
  2. In your experience, how often is it retrying? Since Google follows a (small) fee for service mode, should we be concerned about retries?

@ataustin
Copy link
Author

Interesting point about the fees! The default behavior of RETRY is to attempt only 3 times total before giving up. I'd expect package users to keep hitting the API if their first attempt fails regardless, so RETRY could be a courtesy to them. There is also a terminate_on argument which we could use to specify status codes for which the RETRY would automatically quit.

Let me know if this is acceptable, and if there are any status codes that you would like to kill the RETRY. I'll make any required changes, then update NEWS.md and push.

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.

2 participants