Retry mechanism #577
Retry mechanism #577
Conversation
|
@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. |
|
Adding retry support here seems fine. However, we should not make use of aspect-oriented programming features. Instead, using |
PR feedback - remove AOP and retry-enable ClientHttpRequestFactory
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.
Pushing an update to that effect. |
|
@pmv Thank you for signing the Contributor License Agreement! |
|
Let me know if there's any more legwork you'd like me to do for this one - thanks |
| * | ||
| * @since 3.0.2 | ||
| */ | ||
| public static class Retry { |
There was a problem hiding this comment.
Let's mount retry properties externally, similar to https://github.com/spring-cloud/spring-cloud-config/blob/master/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/RetryProperties.java.
| @@ -129,7 +130,15 @@ protected RestTemplateBuilder restTemplateBuilder(ClientHttpRequestFactory reque | |||
| @Bean | |||
| @ConditionalOnMissingBean | |||
| public ClientFactoryWrapper clientHttpRequestFactoryWrapper() { | |||
| return new ClientFactoryWrapper(this.configuration.createClientHttpRequestFactory()); | |||
There was a problem hiding this comment.
It would make sense to reuse potentially existing RetryTemplate beans (or instances when using the ConfigData boostrapper, see https://github.com/spring-cloud/spring-cloud-config/blob/master/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientRetryBootstrapper.java)
There was a problem hiding this comment.
New changes re-use existing RetryTemplate beans
| * Tests without spring-retry on the classpath to be sure there is no hard dependency | ||
| */ | ||
| @RunWith(ModifiedClassPathRunner.class) | ||
| @ClassPathExclusions({ "spring-retry-*.jar" }) |
There was a problem hiding this comment.
This seemed like an improvement over having a separate module to run tests without spring-retry on the classpath (assuming you're fine with spring-cloud-test-support as a test dependency)
| * {@link VaultReactiveBootstrapConfigurationTests#shouldNotConfigureReactiveSupport()} | ||
| */ | ||
| @Test | ||
| public void shouldNotConfigureReactiveSupport() { |
There was a problem hiding this comment.
This is something I happened to stumble across - perhaps it belongs in a separate PR. I assume you want 'spring.cloud.vault.reactive.enabled' to behave the same across ConfigData & bootstrap configuration.
I added the property to VaultProperties so I could reference it in the ConfigData path - perhaps there is another way if you don't want it in VaultProperties.
|
Let me know if you have any more feedback - I believe suggestions up to this point have been addressed. |
|
Thanks. I want to have a look at this in the upcoming days. |
|
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? |
|
Right now, we don't have bandwidth to follow up timely. |
| First, you need to set `spring.cloud.vault.fail-fast=true`. | ||
| Then you need to add `spring-retry` to your classpath. | ||
| The default behavior is to retry six times with an initial backoff interval of 1000ms and an exponential multiplier of 1.1 for subsequent backoffs. | ||
| You can configure these properties by setting the `spring.cloud.config.retry.*` configuration properties. |
There was a problem hiding this comment.
This should read spring.cloud.vault.retry.* instead?
|
@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! |
|
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. |
…retry # Conflicts: # spring-cloud-vault-config/src/main/java/org/springframework/cloud/vault/config/VaultConfigDataLoader.java
|
Hi guys: Are you planing to merge this branch? The future seems to be great! Thanks |
|
Hi |
Opening for discussion - I believe vault is critical enough infrastructure that clients should have a retry mechanism out of the box.
Relates to spring-projects/spring-vault#504
and
#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