1996fanrui commented on code in PR #710:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/710#discussion_r1393545577
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/ConfigMapStore.java:
##########
@@ -44,13 +43,44 @@ public class ConfigMapStore {
private final KubernetesClient kubernetesClient;
- // The cache for each resourceId may be in three states:
- // 1. The resourceId doesn't exist : ConfigMap isn't loaded from
kubernetes, or it's deleted
- // 2 Exists, Optional.empty() : The ConfigMap doesn't exist in Kubernetes
- // 3. Exists, Not Empty : We have loaded the ConfigMap from kubernetes, it
may not be the same
- // if not flushed already
- private final ConcurrentHashMap<ResourceID, Optional<ConfigMap>> cache =
- new ConcurrentHashMap<>();
+ static class ConfigMapState {
+ private boolean flushed = true;
+ private boolean exists = true;
+
+ @VisibleForTesting ConfigMap configMap;
Review Comment:
Great work, Max! 👍
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/ConfigMapStore.java:
##########
@@ -44,13 +43,44 @@ public class ConfigMapStore {
private final KubernetesClient kubernetesClient;
- // The cache for each resourceId may be in three states:
- // 1. The resourceId doesn't exist : ConfigMap isn't loaded from
kubernetes, or it's deleted
- // 2 Exists, Optional.empty() : The ConfigMap doesn't exist in Kubernetes
- // 3. Exists, Not Empty : We have loaded the ConfigMap from kubernetes, it
may not be the same
- // if not flushed already
- private final ConcurrentHashMap<ResourceID, Optional<ConfigMap>> cache =
- new ConcurrentHashMap<>();
+ static class ConfigMapState {
+ private boolean flushed = true;
+ private boolean exists = true;
+
+ @VisibleForTesting ConfigMap configMap;
Review Comment:
Great work, Max! 👍
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/state/ConfigMapStore.java:
##########
@@ -44,13 +43,13 @@ public class ConfigMapStore {
private final KubernetesClient kubernetesClient;
- // The cache for each resourceId may be in three states:
- // 1. The resourceId doesn't exist : ConfigMap isn't loaded from
kubernetes, or it's deleted
- // 2 Exists, Optional.empty() : The ConfigMap doesn't exist in Kubernetes
- // 3. Exists, Not Empty : We have loaded the ConfigMap from kubernetes, it
may not be the same
- // if not flushed already
- private final ConcurrentHashMap<ResourceID, Optional<ConfigMap>> cache =
- new ConcurrentHashMap<>();
+ // The cache for each resourceId may be in four states:
+ // 1. No cache entry: ConfigMap isn't loaded from kubernetes, or it's
deleted.
+ // 2. Cache entry, not created : The ConfigMap doesn't exist in Kubernetes.
+ // 3. Cache entry, not flushed : The ConfigMap exists in Kubernetes, but
it is not updated yet.
+ // 4. Cache entry, flushed and created : We have loaded the ConfigMap from
kubernetes, and it's
+ // up-to-date.
Review Comment:
Would you mind updating these comments based on the latest
`ConfigMapView.State`? For example, using `NEEDS_CREATE` instead of `not
created`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
