search

spring-cloud / spring-cloud-vault

Configuration Integration with HashiCorp Vault
http://cloud.spring.io/spring-cloud-vault/
Apache License 2.0
274 stars 152 forks source link

Retry mechanism #577

Open pmv opened 3 years ago

pmv commented 3 years ago

Opening for discussion - I believe vault is critical enough infrastructure that clients should have a retry mechanism out of the box.

Relates to https://github.com/spring-projects/spring-vault/issues/504 and https://github.com/spring-cloud/spring-cloud-vault/issues/236

Due to the similarities (both bootstrap property sources), default retry properties were set to match spring cloud config's defaults (https://cloud.spring.io/spring-cloud-config/multi/multi__spring_cloud_config_client.html#config-client-retry)

I have not added tests - I can/will do so once a maintainer indicates this PR on the right track and has potential to be merged.

Thank you

pivotal-issuemaster commented 3 years ago

@pmv Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

mp911de commented 3 years ago

Adding retry support here seems fine. However, we should not make use of aspect-oriented programming features. Instead, using RetryTemplate directly is much more efficient. Actually, the ideal variant would be a Retry-enabled ClientHttpRequestFactory.

pmv commented 3 years ago

Adding retry support here seems fine. However, we should not make use of aspect-oriented programming features.

Originally I did not until I saw this: https://github.com/spring-cloud/spring-cloud-config/blob/master/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServiceBootstrapConfiguration.java#L62-L78, and then I modeled my changes after those. However, I am not set on that particular implementation.

Actually, the ideal variant would be a Retry-enabled ClientHttpRequestFactory.

Pushing an update to that effect.

pivotal-issuemaster commented 3 years ago

@pmv Thank you for signing the Contributor License Agreement!

pmv commented 3 years ago

Let me know if there's any more legwork you'd like me to do for this one - thanks

pmv commented 3 years ago

Let me know if you have any more feedback - I believe suggestions up to this point have been addressed.

mp911de commented 3 years ago

Thanks. I want to have a look at this in the upcoming days.

stefan-g commented 2 years ago

Hi, are there any timelines when retry will be supported? Is there a workaround example how to setup retry for spring-cloud-vault.3.0.3?

mp911de commented 2 years ago

Right now, we don't have bandwidth to follow up timely.

krisiye commented 2 years ago

@mp911de - It will be great if this can be reviewed and supported for a subsequent release as it could save us from implementing custom retry solutions within every vault client app. Thanks!

juriohacc commented 2 years ago

Iam agree, it will be be nice if this feature is implemented, the usage of vault for database access is very critical.

For the moment, if someone could share a full example of code workaround to handle retry (without AOP and forking the repository is better) it will be nice.

Thank you.

iozho commented 1 year ago

Hi guys:

Are you planing to merge this branch? The future seems to be great!

Thanks

desprez commented 1 year ago

Hi When you plan to merge this ?