feat(retry): allow customized retry policy

[dentiny]

Which issue does this PR close?

• Closes Feature request: allow customized retry policy #840

Rationale for this change

Different storage backends have different behaviors -- not all of them conform the "common sense" of HTTP status code, so I think deciding whether to retry based on the status code and message is beneficial.

What changes are included in this PR?

This PR abstracts a retry policy, so users could customize with own policy.

Are there any user-facing changes?

Yes.

[peterxcli]

I think we could give RetryConfig two functions — an internal one that calls another function that can be overridden/customized — so we can move the default behavior into RetryConfig. We could also simplify should_retry_status to just if let Some(policy) => policy, or even inline that and get rid of the new should_retry_status function entirely.

[Xuanwo]

How about we adopt backon 👻

[alamb]

since this is new field in a struct with all public fields, this is breaking API change

[dentiny]

Yes, it's intentional :)

[alamb]

How about we adopt backon 👻

I would rather allow people to adopt whatever API they want :)

[alamb]

I think this sounds like a reasonable idea -- thank you @dentiny -- this would be nice to include in the next major release though I am not sure when that would be

• Release object store 0.15.0 (breaking) - Target TBD #762

I haven't looked at the code closely, but it sounds like @peterxcli has some good suggestions.

[dentiny]

I think this sounds like a reasonable idea -- thank you @dentiny -- this would be nice to include in the next major release though I am not sure when that would be

• Release object store 0.15.0 (breaking) - Target TBD #762

I haven't looked at the code closely, but it sounds like @peterxcli has some good suggestions.

Thanks for the review! I will address the comments later today.

[dentiny]

How about we adopt backon 👻

Thanks for the notice and comment! I mentioned it as alternative solution in the feature request issue, but thought it'd be a better change -- would like to focus some scoped improvement first, backon could be a separate issue and PR if maintainers are happy with it.

[dentiny]

@peterxcli Hey I moved the default behavior into default_should_retry_status based on your suggestion and kept the small should_retry_status util to centralize custom-policy. Would appreciate if you could take another look.

[peterxcli]

LGTM, thanks for the update!

[tustvold]

I'm a little lukewarm on this as it extends the existing retry policy introducing a breaking change, whilst also being relatively limited. I feel if we want to allow customisation of retry functionality we'd be better off making the whole retry function pluggable so people can choose different back off logic, etc...

For the specific case in the ticket I think you could just provide an HttpClient wrapper that maps the status codes to the appropriate HttpError?

[dentiny]

I feel if we want to allow customisation of retry functionality we'd be better off making the whole retry function pluggable so people can choose different back off logic, etc...

I agree that, if we are going to introduce a breaking change, making the entire retry mechanism pluggable would provide a more complete extension point.

One possible approach would be to adopt backon, which would provide configurable retry and backoff behavior instead of introducing another custom abstraction. There has already been some discussion about this above (1, 2).

Would you prefer adopting backon, or exposing a more general pluggable retry interface that does not require users to adopt a specific retry library?

[tustvold]

I left some thoughts on #831 (comment)

IMO we already have an HttpService abstraction that encapsulates how HTTP requests are made, it would seem ideal to me if retries would just be a layer in an HttpService stack.

As for adding an explicit dependency on a retry library, I am generally pretty lukewarm on adding additional dependencies unless strictly necessary - I am not sure this passes that bar.

[alamb]

IMO we already have an HttpService abstraction that encapsulates how HTTP requests are made, it would seem ideal to me if retries would just be a layer in an HttpService stack.

I agree -- this would help users as well so they could extend out the retry mechanism with whatever policy they wanted, rather than be limited to what was provided in the library

[criccomini]

I took a crack at a very limited RetryPolicy that specifically addresses #831:

#849

A subsequent PR could theoretically use the RetryPolicy trait for the HttpService layer as well (by adding a RetryService and AmazonS3Builder::with_retry_policy, etc).