PatrickRen commented on code in PR #20734:
URL: https://github.com/apache/flink/pull/20734#discussion_r969312548
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/keyselector/GenericRowDataKeySelector.java:
##########
@@ -44,12 +44,16 @@ public GenericRowDataKeySelector(
this.keySerializer = keySerializer;
}
+ public void open() {
Review Comment:
Is there any specific reason to extract an open method here?
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/table/lookup/fullcache/CacheLoader.java:
##########
@@ -121,8 +128,15 @@ public void run() {
@Override
public void close() throws Exception {
- if (cache != null) {
- cache.clear();
+ isStopped = true;
+ stopLoading();
+ reloadLock.lock(); // if reload is in progress, we will wait until it
is over
Review Comment:
Is there any chance to interrupt the reloading task directly on close
instead of waiting for the reload to finish?
Also I think `stopLoading` is just a step of closing the loader, so it's a
bit weird to create a new abstract method instead of overriding the close in
the implementation.
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/table/lookup/fullcache/LookupFullCache.java:
##########
@@ -72,6 +72,13 @@ public synchronized void open(Configuration parameters)
throws Exception {
} else {
reloadFailCause.addSuppressed(th);
}
+ try {
Review Comment:
I think once an error is encountered in the cache loader, the exception will
finally be thrown in the `getIfPresent` method and the cache will be closed
eventually. Is there any reason to explicitly close the cache here?
--
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
