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

Has re-added code for using value of "retry-after" header as waiting time if status_code = 429 #472

Merged
merged 3 commits into from Aug 4, 2017

Conversation

@nielsoledam
Copy link
Contributor

@nielsoledam nielsoledam commented Aug 4, 2017

I've reset my fork to the current master and re-added the code changes as discussed on #434.

The code now also respects the value of pause_min.

…time if status_code = 429. Now also respects value of pause_min.
Copy link
Member

@hadley hadley left a comment

Can you please also add a note to the documentation (mentioning that it understand the 429 status code), and add a bullet to news following the existing template.

R/retry.R Outdated
@@ -91,6 +91,10 @@ backoff_full_jitter <- function(i, resp, pause_base = 1, pause_cap = 60,
error_description <- ""
status <- status_code(resp)
}
if (status == 429) {
length_429 <- as.numeric(resp$headers[["retry-after"]])
if (!is.null(length_429) && length_429 >= pause_min) length <- length_429
Copy link
Member

@hadley hadley Aug 4, 2017

I don't think the logic is quite right here - length_429 will never be NULL because you coerce it to numeric

Inside the if statement, I'd prefer something like length <- max(pause_min, length_429)

hadley
hadley approved these changes Aug 4, 2017
Copy link
Member

@hadley hadley left a comment

Looks good. Just one last small tweak

R/retry.R Outdated
length_429 <- as.numeric(resp$headers[["retry-after"]])
if (!is.null(length_429) && length_429 >= pause_min) length <- length_429
retry_after <- resp$headers[["retry-after"]]
if (!is.null(retry_after)) length <- max(pause_min, as.numeric(retry_after))
Copy link
Member

@hadley hadley Aug 4, 2017

Can you please put the condition code inside of {} (indented like the rest of the code)

Copy link
Contributor Author

@nielsoledam nielsoledam Aug 4, 2017

Was this what you meant?

@hadley hadley merged commit 9af8acd into r-lib:master Aug 4, 2017
1 of 3 checks passed
@hadley
Copy link
Member

@hadley hadley commented Aug 4, 2017

Perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants