Skip to content

Add maxNetworkRetries as a global and per-request setting#934

Merged
ob-stripe merged 1 commit intointegration-client-refactorfrom
ob-maxnetworkretries
Jan 15, 2020
Merged

Add maxNetworkRetries as a global and per-request setting#934
ob-stripe merged 1 commit intointegration-client-refactorfrom
ob-maxnetworkretries

Conversation

@ob-stripe
Copy link
Contributor

r? @brandur-stripe @richardm-stripe
cc @stripe/api-libraries

I added support for automatic request retries back in #900, but the setting for the max number of retries was set on the HttpClient instance, which users won't be initializing themselves directly in most cases.

In order to make the feature more accessible, this PR changes the setting so that it can be set in one of two ways:

  • per request, via RequestOptions.builder().setMaxNetworkRetries(2).build()
  • globally, via Stripe.setMaxNetworkRetries(2)

(The former takes precedence over the latter.)

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

WFM, but I'd point out that conceptually, having this as a property that's configured per-client makes more sense to me than one that's configured per-request. e.g. I could for example configure one client for api.stripe.com with 3 retries and another for localhost:12111 (stripe-mock) with 0 retries since it's local and unlikely to fail.

That said, I know that stripe-java's HttpClient doesn't really work that way, but if it was more like a client.API like in stripe-go or a StripeClient like in stripe-ruby, per-client would make more sense.

Either way though, agreed that it should be stripped out of the HttpClient constructor (in favor of a setter).

Don't feel too strongly about this though, so will leave it up to you!

LGTM.

@ob-stripe
Copy link
Contributor Author

Thanks Brandur!

I agree that setting this property per-request is a bit unorthodox, but it's consistent with what we're doing already for e.g. connect and read timeouts. The main benefit is that RequestOptions is something that stripe-java users are already familiar with, so this makes the feature a lot more discoverable (as opposed to creating a custom HttpClient, which nobody is familiar with because this was introduced in this integration branch).

I plan on reintroducing this setting as a property on the client when we make more progress towards the client + services architecture, and it becomes more commonplace for users to instantiate clients directly.

@ob-stripe ob-stripe merged commit 77e1087 into integration-client-refactor Jan 15, 2020
@ob-stripe ob-stripe deleted the ob-maxnetworkretries branch January 15, 2020 00:37
@brandur-stripe
Copy link
Contributor

Works for me. Thanks OB.

ob-stripe added a commit that referenced this pull request Jan 15, 2020
* Refactor form encoding

* Refactor request telemetry

* Move HTTP request methods into new `HttpClient` class

* Add `StripeRequest` object

* Add `HttpClient` abstract class

* Stop disabling the DNS cache

* Fix deprecation warnings (#895)

* Add HttpContent class (#896)

* Add Stopwatch class (#897)

* Move all request properties in `StripeRequest` (#898)

* Remove ApiResource.RequestType (#899)

* Add support for automatic request retries (#900)

* Minor fixes (#902)

* `StringUtils` class & better API key validation (#928)

* Remove support for custom `URLStreamHandler` (#927)

* Refactor HTTP headers handling (#931)

* Add `CaseInsensitiveMap` class

* Add `HttpHeaders` class

* Use `HttpHeaders` in `StripeRequest`

* Use `HttpHeaders` in `StripeResponse`

* Address review comments

* Modernize `StripeResponse` (#932)

* Add `maxNetworkRetries` as a global and per-request setting (#934)

* Add `StreamUtils` class (#935)

* Remove support for `count` and `total_count` in list objects (#936)

* Codegen for openapi e07de1a (#938)

* Update README (#939)

Co-authored-by: remi-stripe <remi@stripe.com>
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