zentol commented on code in PR #20919: URL: https://github.com/apache/flink/pull/20919#discussion_r1037128521
########## flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/table/lookup/fullcache/inputformat/InputFormatCacheLoader.java: ########## @@ -107,7 +109,11 @@ protected void reloadCache() throws Exception { } catch (InterruptedException ignored) { // we use interrupt to close reload thread } finally { if (cacheLoadTaskService != null) { + // if main cache reload thread encountered an exception, + // it interrupts underlying InputSplitCacheLoadTasks threads cacheLoadTaskService.shutdownNow(); Review Comment: I'm not sure what the question is. We certainly shouldn't use the common fork pool. I can see what caused that decision to be made, but I'd say the wrong conclusion was drawn. reloadCache should've explictly been an async operation. That would also make it obvious when looking at the class where concurrency actually exists. If this operation is so heavy that you don't want to call it in the calling thread then it probably shouldn't run in the common pool. Not a huge fan of repeatedly creating a new executor service btw; I'd rather create it once for a consistent resource profile. As a side-effect it would then also be trivial to make this call async. -- 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