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

Default number of network retries to 2 #1069

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Apr 16, 2020

Currently, it's possible to have the library retry intermittently failed
requests by configuring MaxNetworkRetries on a BackendConfig.
Because this is a pretty useful feature that many users will never
discover because it's off by default, we've been mulling around the idea
internally to change that default on the next major so that more people
get access to it. We're about to release V71, so now is an opportune
moment.

The slight complication is that BackendConfig.MaxNetworkRetries is
currently a simple int, which means that it's hard for us to recognize
an unset value versus an explicitly set 0 (the same problem we had with
fields on parameter structs for years). So by example, this code is
problematic:

if conf.MaxNetworkRetries == 0 {
    backend.MaxNetworkRetries = 2
}

The most obvious solution is that change MaxNetworkRetries to a
nilable pointer, and have it set using our stripe.Int64 helper,
exactly as we do for parameter structs. So compared to today,
configuring it would change like this:

 config := &stripe.BackendConfig{
-    MaxNetworkRetries: 2,
+    MaxNetworkRetries: stripe.Int64(2),
 }

It's not too bad, and follows convention found elsewhere in the library,
but will require a small code update for users.

The slight follow on complication is that to make BackendConfig
self-consistent, I also changed the other primitives on it to also be
pointers, so EnableTelemetry changes from bool to *bool and URL
changes from string to *string. I don't think this is a big deal
because ~99% of users will probably just be using the defaults by having
left them unset.

r? @ob-stripe @remi-stripe A little more complicated than I was
originally hoping, but not too bad. I still think we should do it.
Thoughts?
cc @stripe/api-libraries


Note: Targets the branch in #1055 for V71 integration.

@brandur brandur mentioned this pull request Apr 16, 2020
7 tasks
@brandur-stripe
Copy link
Contributor

(Also, if you guys think this is okay, I'll update the README. I didn't bother for now.)

Currently, it's possible to have the library retry intermittently failed
requests by configuring `MaxNetworkRetries` on a `BackendConfig`.
Because this is a pretty useful feature that many users will never
discover because it's off by default, we've been mulling around the idea
internally to change that default on the next major so that more people
get access to it. We're about to release V71, so now is an opportune
moment.

The slight complication is that `BackendConfig.MaxNetworkRetries` is
currently a simple `int`, which means that it's hard for us to recognize
an unset value versus an explicitly set 0 (the same problem we had with
fields on parameter structs for years). So by example, this code is
problematic:

``` go
if conf.MaxNetworkRetries == 0 {
    backend.MaxNetworkRetries = 2
}
```

The most obvious solution is that change `MaxNetworkRetries` to a
nilable pointer, and have it set using our `stripe.Int64` helper,
exactly as we do for parameter structs. So compared to today,
configuring it would change like this:

``` patch
 config := &stripe.BackendConfig{
-    MaxNetworkRetries: 2,
+    MaxNetworkRetries: stripe.Int64(2),
 }
```

It's not too bad, and follows convention found elsewhere in the library,
but will require a small code update for users.

The slight follow on complication is that to make `BackendConfig`
self-consistent, I also changed the other primitives on it to also be
pointers, so `EnableTelemetry` changes from `bool` to `*bool` and `URL`
changes from `string` to `*string`. I don't think this is a big deal
because ~99% of users will probably just be using the defaults by having
left them unset.
@brandur-stripe
Copy link
Contributor

Thanks OB! Rebased on rewrote the appropriate section in the README.

@brandur-stripe brandur-stripe merged commit 74f3fd0 into integration-v71 Apr 17, 2020
@brandur-stripe brandur-stripe deleted the brandur-default-2-retries branch April 17, 2020 17:40
nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
* enable turning off data mapper fields

* comment out console.log calls. Comments are scrubbed on deploy so they're fine to leave in the code.

* Removed debugging overrides
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