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

Clarify that automatic network retries occur for most api errors #567

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

rattrayalex-stripe
Copy link
Contributor

r? @remi-stripe
cc @paulasjes-stripe
cc @brandur-stripe
cc @stripe/api-libraries
Follows #566 which follows #559

README.md Outdated
@@ -147,7 +147,7 @@ if (process.env.http_proxy) {

### Network retries

Automatic network retries can be enabled with `setMaxNetworkRetries`. This will retry requests `n` times with exponential backoff if they fail due to connection or conflict errors. [Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication.
Automatic network retries can be enabled with `setMaxNetworkRetries`. This will retry requests `n` times with exponential backoff if they fail due to connection errors or most api errors. [Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "most api errors" imply? It seems wrong. The most important API errors are card_error and invalid_request_error which we would explicitly never retry right?

Copy link
Contributor

@paulasjes-stripe paulasjes-stripe Feb 14, 2019

Choose a reason for hiding this comment

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

I'd be more explicit here and list the exact errors we do don't retry on to reduce any confusion.

Copy link
Contributor Author

@rattrayalex-stripe rattrayalex-stripe Feb 14, 2019

Choose a reason for hiding this comment

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

to wit:

Automatic network retries can be enabled with setMaxNetworkRetries. This will retry requests n times with exponential backoff if they fail due to connection errors, 409 (conflict) errors, 503 (availability) errors, and 500 (internal) errors on GET/DELETE. Idempotency keys are added where appropriate to prevent duplication.

400 (user) errors and 429 (rate limit exceeded) errors are not retried, nor are 500 errors which result from POST requests, as our idempotency framework would simply no-op and return a replayed 500 error.

eh?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally remove the last sentence, it seems quite confusing. Or we need to get into the weeds of what is retry-able and what isn't which seems out of scope from this small section in the readme

@rattrayalex-stripe
Copy link
Contributor Author

updated to match php/python, which I think expresses the intent better.

r? @remi-stripe

@rattrayalex-stripe rattrayalex-stripe merged commit b209c82 into master Feb 14, 2019
@ob-stripe ob-stripe deleted the rattrayalex-stripe-patch-1 branch October 29, 2019 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants