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

Add support for retries #613

Closed
wants to merge 2 commits into from
Closed

Conversation

worldtiki
Copy link

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
Copy link
Member

mp911de commented Jan 13, 2021

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
Copy link
Author

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?

image

@mp911de
Copy link
Member

mp911de commented Jan 14, 2021

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
Copy link
Author

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: spring-cloud/spring-cloud-vault@master...worldtiki:retries

@pmv
Copy link

pmv commented Mar 2, 2021

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
Copy link
Member

mp911de commented Mar 3, 2021

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

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

@mp911de mp911de closed this Mar 3, 2021
@mp911de mp911de added the for: external-project For an external project and not something we can fix label Mar 3, 2021
@worldtiki
Copy link
Author

That's alright. Thanks for the update,

@pmv feel free to reuse bits from spring-cloud/spring-cloud-vault@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
Copy link
Member

mp911de commented Mar 3, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Retry mechanism for vault template
3 participants