[GitHub] [flink] zentol commented on a diff in pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest

[GitBox <gi...@apache.org>]

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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org