Add support for retries

[worldtiki]

Fixes #504.

This is just an initial draft to get feedback. There's no tests and it only has the retry in one (doWithSession) of several methods.

This would then need a second pr for spring cloud vault (here) so that we can use the spring.cloud.vault.something.retries=3 niceness.

I struggled a bit about where to setup the retry template. Is that setter a good idea? I'm a bit confused about the optional scope of some dependencies too, and even if spring retry is needed or, given how simple this is, we could just wrap this in some loop / try catch and not add extra libraries.

[mp911de]

This change introduces a mandatory dependency on Spring Retry. Instead, it would make sense to have a ClientHttpRequestFactory that uses the Retry Template, ideally, within Spring Retry itself since HTTP retries aren't specific to Spring Vault.

[worldtiki]

Thanks for the quick reply Mark!

I'm a bit confused by the last part of the comment, the "ideally within spring retry itself".

A new factory would look like this, no?

[mp911de]

While it requires a new factory, users need to be able to specify a bit of configuration so we can't provide a context-less factory. It would make sense to plug the Retry integration on configuration-level together in the sense, if there's Spring Retry on the class path and a RetryTemplate is available, then we would pick up the ClientHttpRequestFactory and pass on the correct ClientHttpRequestFactory to RestTemplate and VaultTemplate.

[worldtiki]

I took a stab at this but with the injection of the retry template things get a bit messy. I reckon folks using this project would be ok with a solution a bit more verbose like in your answer here.

So I wonder if it would make sense to move this to spring cloud vault instead. Imo it's where we'd get more return on investment by having a more concise solution (ie, we could use spring.cloud.value.retries=x). There's still the issue of the dependency on retry that's not something that's straightforward to solve but this would be contained in a single project since we can create/override the ClientHttpRequestFactory there. Similar to this: https://github.com/spring-cloud/spring-cloud-vault/compare/master...worldtiki:retries

[pmv]

Thoughts on the last comment, @mp911de? My organization is running into this also - if a PR to spring-cloud-vault is a better location for this I'm willing to submit something for feedback.

[mp911de]

We couldn't convince ourselves that duplicating efforts regarding HTTP retries is a good idea across projects. Closing this PR in favor of https://github.com/spring-cloud/spring-cloud-vault/pull/577.

Leveraging retry functionality of the supported Apache HTTP Client would be a much better option (see https://memorynotfound.com/apache-httpclient-httprequestretryhandler-example/).

[worldtiki]

That's alright. Thanks for the update,

@pmv feel free to reuse bits from https://github.com/spring-cloud/spring-cloud-vault/compare/master...worldtiki:retries

I think the only thing missing is that it's using spring-retry and not HttpRequestRetryHandler. The example @mp911de shared looks simple enough, it would just be a matter of adding some sort of backoff.

[mp911de]

Don't get me wrong, if you're able to use Apache HTTP Client, then that would be the easiest approach. If you cannot use Apache HTTP Client and are required to use the JDK Client or Netty, then there's no built-in retry functionality.