[GitHub] [tez] zeroflag commented on a change in pull request #75: TEZ-4213: Bound appContext executor capacity

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

zeroflag commented on a change in pull request #75:
URL: https://github.com/apache/tez/pull/75#discussion_r464869904



##########
File path: tez-dag/src/main/java/org/apache/tez/dag/app/DAGAppMaster.java
##########
@@ -621,7 +621,7 @@ public synchronized void serviceInit(final Configuration conf) throws Exception
       }
     }
 
-    Executors.newFixedThreadPool(conf.getInt(TezConfiguration.TEZ_AM_DAG_APPCONTEXT_THREAD_COUNT_LIMIT,
+    rawExecutor = Executors.newFixedThreadPool(conf.getInt(TezConfiguration.TEZ_AM_DAG_APPCONTEXT_THREAD_COUNT_LIMIT,

Review comment:
       The newFixedThreadPool() still returns an executor with an unbounded queue (actually it's bounded but the  max capacity of the LinkedBlockingQueue is Integer.MAX_VALUE so practically it's unbounded). 
   
   It won't spin up more threads than specified (100) but if all threads are working then it'll start queueing up tasks in a queue which will practically never be full. Doesn't this still cause OOM?
   
   What would be the desired behaviour when the queue is full? Rejection? Exception or blocking?




----------------------------------------------------------------
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.

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