replaced single-shot HTTP requests with RETRY logic #235
Conversation
R/io.R
Outdated
| @@ -258,7 +258,7 @@ write_civis.character <- function(x, tablename, database = NULL, if_exists = "fa | |||
| credential_id = credential_id), | |||
| import_args) | |||
| job_r <- do.call(start_import_job, args) | |||
| put_r <- httr::PUT(job_r[["uploadUri"]], body = httr::upload_file(x)) | |||
| put_r <- httr::RETRY("PUT", job_r[["uploadUri"]], body = httr::upload_file(x), terminate_on = c(403, 404)) | |||
There was a problem hiding this comment.
I think it'd be better to terminate on any 4xx error other 429 here (e.g., terminate_on = setdiff(400:499, 429)). That's what AWS recommends here, and this URI and the one for write_disk below will be S3 URLs.
There was a problem hiding this comment.
sure I can do that! setdiff() is a super clean way to express it, I like it 😀
There was a problem hiding this comment.
addressed the comments in 7cdb800
R/io.R
Outdated
| list(httr::write_disk(file, overwrite = overwrite))) | ||
| list(httr::write_disk(file, overwrite = overwrite)), | ||
| verb = "GET", | ||
| terminate_on = c(403, 404)) |
There was a problem hiding this comment.
Could you also please make this terminate_on = setdiff(400:499, 429)? Sorry, I should have commented about each case rather than just the first.
There was a problem hiding this comment.
oh sure, no problem!
R/io.R
Outdated
| @@ -902,14 +904,14 @@ download_script_results <- function(script_id, run_id, | |||
| stop_if_no_output(script_results) | |||
|
|
|||
| url <- script_results[["output"]][[1]][["path"]] | |||
| args <- list(url) | |||
| args <- list(url, verb = "GET", terminate_on = c(403, 404)) | |||
There was a problem hiding this comment.
Could you also please make this terminate_on = setdiff(400:499, 429)?
|
I pushed #235 to address the most recent review comments |
|
@jameslamb, would you care to merge this? (no rush, I just noticed this was still open) |
@mheilman I don't have permission to merge pull requests in this repository. That has to be done by a maintainer.
|
|
Oh, my bad, sorry. I forgot about that! I thought you could merge if it was approved by a maintainer or something. |
In this PR, I'd like to propose swapping out calls to
httr::POST()etc. withhttr::RETRY(). It looks like you're already usingRETRY()in a lot of places, which is awesome! This PR just changes the few remaining places where you weren't.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.
I also noticed that you are mocking
httr::VERB()but that it's never called in the code, so I removed those mocks.I'm working on chircollab/chircollab20#1 as part of Chicago R Collab, an R 'unconference' in Chicago.
Thanks for your time and consideration!