1996fanrui commented on code in PR #710:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/710#discussion_r1391225899
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/ConfigMapStore.java:
##########
@@ -67,19 +67,14 @@ protected Optional<String> getSerializedState(
}
protected void removeSerializedState(KubernetesJobAutoScalerContext
jobContext, String key) {
- getConfigMap(jobContext)
- .ifPresentOrElse(
- configMap -> configMap.getData().remove(key),
- () -> {
- throw new IllegalStateException(
- "The configMap isn't created, so the
remove is unavailable.");
- });
+ getConfigMap(jobContext).ifPresent(configMap ->
configMap.getData().remove(key));
}
public void flush(KubernetesJobAutoScalerContext jobContext) {
Optional<ConfigMap> configMapOpt = cache.get(jobContext.getJobKey());
if (configMapOpt == null || configMapOpt.isEmpty()) {
LOG.debug("The configMap isn't updated, so skip the flush.");
+ // Do not flush if there are no updates.
Review Comment:
I write 2 tests:
1. one is `testRemoveAndFlushEveryTime`, it's similar to current PR code
2. one is `testQueryAndFlushIfNeeded`, it's similar to master code
- `testRemoveAndFlushEveryTime` will remove and flush every time, and it
will call kubernetes every time.
- `testQueryAndFlushIfNeeded` will query from cache, and value is null, so
remove and flush isn't necessary. So it doens't call kubernetes.
My question is: the extra call is expected? And I think this PR is fine if
it doesn't introuduce any extra opration. Please correct my if my understanding
is wrong, thanks a lot.
These 2 tests can be added to ConfigMapStoreTest, and they can run directly.
```
@Test
void testRemoveAndFlushEveryTime() {
KubernetesJobAutoScalerContext ctx = createContext("cr1",
kubernetesClient);
var key = "key";
var value = "value";
var configMapStore = new ConfigMapStore(kubernetesClient);
configMapStore.putSerializedState(ctx, key, value);
assertEquals(2, mockWebServer.getRequestCount());
configMapStore.flush(ctx);
assertEquals(3, mockWebServer.getRequestCount());
// Get from cache
assertThat(configMapStore.getSerializedState(ctx,
key)).hasValue(value);
assertEquals(3, mockWebServer.getRequestCount());
// Get from kubernetes
configMapStore = new ConfigMapStore(kubernetesClient);
assertThat(configMapStore.getSerializedState(ctx,
key)).hasValue(value);
assertEquals(4, mockWebServer.getRequestCount());
// Remove the key
configMapStore.removeSerializedState(ctx, key);
assertEquals(4, mockWebServer.getRequestCount());
configMapStore.flush(ctx);
assertEquals(5, mockWebServer.getRequestCount());
// Every flush will call kubernetes.
configMapStore.flush(ctx);
assertEquals(6, mockWebServer.getRequestCount());
configMapStore.flush(ctx);
assertEquals(7, mockWebServer.getRequestCount());
configMapStore.flush(ctx);
assertEquals(8, mockWebServer.getRequestCount());
}
@Test
void testQueryAndFlushIfNeeded() {
KubernetesJobAutoScalerContext ctx = createContext("cr1",
kubernetesClient);
var key = "key";
var value = "value";
var configMapStore = new ConfigMapStore(kubernetesClient);
configMapStore.putSerializedState(ctx, key, value);
assertEquals(2, mockWebServer.getRequestCount());
configMapStore.flush(ctx);
assertEquals(3, mockWebServer.getRequestCount());
// Get from cache
assertThat(configMapStore.getSerializedState(ctx,
key)).hasValue(value);
assertEquals(3, mockWebServer.getRequestCount());
// Get from kubernetes
configMapStore = new ConfigMapStore(kubernetesClient);
assertThat(configMapStore.getSerializedState(ctx,
key)).hasValue(value);
assertEquals(4, mockWebServer.getRequestCount());
// Remove the key
configMapStore.removeSerializedState(ctx, key);
assertEquals(4, mockWebServer.getRequestCount());
configMapStore.flush(ctx);
assertEquals(5, mockWebServer.getRequestCount());
// get is null, so don't flush
configMapStore.getSerializedState(ctx, key);
assertEquals(5, mockWebServer.getRequestCount());
configMapStore.getSerializedState(ctx, key);
assertEquals(5, mockWebServer.getRequestCount());
configMapStore.getSerializedState(ctx, key);
assertEquals(5, mockWebServer.getRequestCount());
}
```
--
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
