Feature/retry policy configmap property source #446
Conversation
| } | ||
|
|
||
| public ConfigMapPropertySource(KubernetesClient client, String name, String namespace, | ||
| String[] profiles) { | ||
| this(client, name, namespace, createEnvironmentWithActiveProfiles(profiles)); | ||
| RetryPolicy retryPolicy, String[] profiles) { |
There was a problem hiding this comment.
I would prefer we not change the public constructor and instead create a new one (mark these two as deprecated)
| + configMapMessage); | ||
| throw kce; | ||
| } | ||
| Thread.sleep(normRetryPolicy.getDelay()); |
There was a problem hiding this comment.
@geoand @iocanel @spencergibb thoughts on this? Is there any other way we could do this?
There was a problem hiding this comment.
My first thought was to include spring-retry but I rejected it for the simple reason that not to include a new dependency. What do you think about it?
There was a problem hiding this comment.
If we don't mind adding another dependency, we have used: https://github.com/awaitility/awaitility in other projects and it's pretty great.
One caveat is that I have only used for tests, so not sure what the implications are of adding it as a regular dependency
There was a problem hiding this comment.
We have use Spring Retry in other Spring Cloud projects so I would be in favor of that
|
I don't understand the error in CI flow. Sorry if I have a misconception. Should I submit the feature against the 1.0.x stream? |
|
The failure is indeed very weird. I just relaunched the job, let's see what happens |
| * | ||
| * @author Andres Navidad | ||
| */ | ||
| public final class RetryPolicyManager { |
There was a problem hiding this comment.
I would actually prefer if this class was turned into an interface and we provide a default implementation as a Bean and make it @ConditionalOnMissingBean. That way if someone wants to customize this to their liking they can easily do so
…bean. Update README with these changes. Some additional test
|
@anavidad3 @ryanjbaxter is there any progress on this? I'm currently trying to use SCK-ConfigMap and it doesn't work more than it does (80% failure rate) in GKE... so without this fix it feels not fit for purpose. Any other tips on how to get this working while I wait for a release with this fix? |
|
Hi @Jumwah. PR is complete with changes requested by @ryanjbaxter. I think it's ready to be merge. |
|
@anavidad3 I will take a look at it, but it probably won't make our Hoxton.SR1 release. |
There was a problem hiding this comment.
Just some high level comments at the moment.
I would like @spencergibb to take a look at this as well
| * maxAttempts: < 0 (infinite retries) | ||
| * maxAttempts: > 1 (number maximum of retries) | ||
|
|
||
| For `spring.cloud.kubernetes.config.retryPolicy.delay` property (in milliseconds): |
There was a problem hiding this comment.
Rather than delay I would be consistent and change this to be backoffpolicy
| @@ -173,6 +177,10 @@ | |||
| <version>${groovy.version}</version> | |||
| <scope>test</scope> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
Why is this in here twice?
| @@ -61,6 +61,10 @@ | |||
| <groupId>org.springframework.security</groupId> | |||
| <artifactId>spring-security-rsa</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
I think this should be optional....ie retry is only going to happen if the app has spring-retry on the classpath
| @@ -29,6 +31,8 @@ | |||
|
|
|||
| protected String namespace; | |||
|
|
|||
| protected RetryPolicy retryPolicy; | |||
There was a problem hiding this comment.
What is this used for?
| @@ -48,12 +54,16 @@ | |||
| @Autowired | |||
| private KubernetesClient client; | |||
|
|
|||
| @Autowired | |||
| private RetryPolicyOperations retryPolicyOperations; | |||
|
|
|||
| @Bean | |||
| @ConditionalOnProperty(name = "spring.cloud.kubernetes.config.enabled", | |||
| matchIfMissing = true) | |||
| public ConfigMapPropertySourceLocator configMapPropertySourceLocator( | |||
There was a problem hiding this comment.
Can we not just pass RetryPolicyOperations in the method signature here?
|
@anavidad3 Are you planning on continuing work on this PR? We are quite eager to have this feature as well, since we want to make sure nodes don't come up with partial configuration. Otherwise, is there another way we can move the PR forward? Happy to assist. |
|
@RoyJacobs feel free to revisit this if you want, for now I am going to close this since we have not heard back from the author. |
What changes were proposed in this pull request?
About issue #441. This pull request offers the possibility of defining a retry policy to read kubernetes configmaps to avoid some ephemeral network problems.
This closes #441