Reload functionality clean-up part 4 #1036
Conversation
wind57
changed the title
| @@ -43,10 +44,10 @@ | |||
| */ | |||
| @Configuration(proxyBeanMethods = false) | |||
| @ConditionalOnKubernetesAndConfigEnabled | |||
| @ConditionalOnClass(EndpointAutoConfiguration.class) | |||
| @ConditionalOnKubernetesReloadEnabled | |||
| @ConditionalOnClass({ EndpointAutoConfiguration.class, RestartEndpoint.class, ContextRefresher.class }) | |||
There was a problem hiding this comment.
these two : RestartEndpoint.class, ContextRefresher.class where part of @ConditionalOnClass, so I just moved them in here, since we dropped the inner class. Initially I put them on @AutoConfigureAfter, but this was a brain fart from my side, as I placed them under the wrong annotation. sorry about that
| @@ -63,32 +64,34 @@ public TaskSchedulerWrapper<TaskScheduler> taskScheduler() { | |||
| @Bean | |||
| @ConditionalOnMissingBean | |||
| public ConfigurationUpdateStrategy configurationUpdateStrategy(ConfigReloadProperties properties, | |||
| ConfigurableApplicationContext ctx, @Autowired(required = false) RestartEndpoint restarter, | |||
| ConfigurableApplicationContext ctx, Optional<RestartEndpoint> restarter, | |||
There was a problem hiding this comment.
use Optional for the optional dependency
| @@ -47,7 +46,9 @@ public class PollingConfigMapChangeDetector extends ConfigurationChangeDetector | |||
|
|
|||
| private final TaskScheduler taskExecutor; | |||
|
|
|||
| private final Duration period; | |||
There was a problem hiding this comment.
minor simplifications here + made a method private
| @@ -58,11 +58,11 @@ | |||
| */ | |||
| @Configuration(proxyBeanMethods = false) | |||
| @ConditionalOnKubernetesAndConfigEnabled | |||
| @ConditionalOnClass(EndpointAutoConfiguration.class) | |||
| @ConditionalOnKubernetesReloadEnabled | |||
| @ConditionalOnClass({ EndpointAutoConfiguration.class, RestartEndpoint.class, ContextRefresher.class }) | |||
There was a problem hiding this comment.
moved RestartEndpoint.class, ContextRefresher.class to @ConditionalOnClass
| @@ -58,11 +58,11 @@ | |||
| */ | |||
| @Configuration(proxyBeanMethods = false) | |||
| @ConditionalOnKubernetesAndConfigEnabled | |||
| @ConditionalOnClass(EndpointAutoConfiguration.class) | |||
| @ConditionalOnKubernetesReloadEnabled | |||
There was a problem hiding this comment.
created a new annotation @ConditionalOnKubernetesReloadEnabled to simplify reading the code
|
|
||
| private KubernetesClient kubernetesClient; | ||
| private SharedIndexInformer<ConfigMap> informer; |
There was a problem hiding this comment.
use informer instead of watcher (just like we do in k8s-native), this will fix an issue I have opened about this subject
| this.watches = new HashMap<>(); | ||
| } | ||
|
|
||
| @PreDestroy |
There was a problem hiding this comment.
we had two @PreDestroy methods, now there is just one; it shutsdown the informer and the client
| */ | ||
| @ConditionalOnProperty("spring.cloud.kubernetes.reload.enabled") | ||
| @ConditionalOnClass({ RestartEndpoint.class, ContextRefresher.class }) | ||
| protected static class ConfigReloadAutoConfigurationBeans { |
There was a problem hiding this comment.
same simplification has been done in fabric8 - drop the inner class, move annotations
| @@ -44,17 +41,15 @@ | |||
| */ | |||
| public class KubernetesClientEventBasedConfigMapChangeDetector extends ConfigurationChangeDetector { | |||
There was a problem hiding this comment.
some minor cleanups in this class
| @@ -114,5 +114,8 @@ | |||
| <module>spring-cloud-kubernetes-client-reactive-discoveryclient-it</module> | |||
| <module>spring-cloud-kubernetes-configuration-watcher-it</module> | |||
| <module>spring-cloud-kubernetes-core-k8s-client-it</module> | |||
| </modules> | |||
| <module>spring-cloud-kubernetes-fabric8-client-configmap-polling-reload</module> | |||
There was a problem hiding this comment.
added 3 more integration tests
| Optional.ofNullable(exception).map(e -> { | ||
| log.debug("Exception received during watch", e); | ||
| return exception.asClientException(); | ||
| }).map(KubernetesClientException::getStatus).map(Status::getCode) |
There was a problem hiding this comment.
I've asked fabric8 team about this HTTP_GONE and they told me that with informers this should not be a problem anymore. Still, if we face it, I've let the commented code in the repo. In future, we will know where to look and how to fix.
There was a problem hiding this comment.
Hmmm was that fixed in a specific k8s version?
There was a problem hiding this comment.
so here is where I asked if we should be handling this in case of informers too and where I was told - not really.
Initially, I thought about adding it via the onNothing method:
@Override
public void onNothing() {
boolean isStoreEmpty = informer.getStore().list().isEmpty();
if(!isStoreEmpty) {
// HTTP_GONE, thus re-inform
inform();
}
}
Marc said that this should not be needed and I am inclined to trust him, since informers are much more resilient than watchers. I am also inclined to trust this statement, since such a method (where you can track a HTTP_GONE) does not even exist in k8s-native implementation.
|
@ryanjbaxter last clean-up on the reload functionality. |
No description provided.