It looks good to me. Thanks! On Mar 3, 2011, at 10:18 PM, ಕರಿಯ wrote:
Hi, Ning 1) You are right on this. But, here the keepAliveTime will not be having much of prominence as the core pool size itself is nThreads and the max pool size is also nThreads. So, even if the threads are idle, nThreads will always remain in the pool that is created to process the tasks. Also, since in this scenario the thread pool is being created for a specific purpose thread pool configuration is fine. This can be achieved in a more simple manner as below: ExecutorService exeService = Executors.newFixedThreadPool(nThreads); I'll use this in the new patch. Further, If we are going to be use (which we need to in future) a common or a centralized thread pool, then the thread pool configuration needs to be carefully arrived at taking into account the number of cores available at our disposal on a particular machine and depending on profiling results, but this is for later. 2) In the current scenario, we need to call execService.shutDown() in any case, if an exception is thrown or otherwise, as it is a local thread pool and we won't be using it any further. If the thread pool were to be a common/centralized one, we need not have to call shutDown(). Please let me know if this is fine, then I'll upload the patch attached with this file in Jira. Thanks, ಕರಿಯ On Fri, Mar 4, 2011 at 12:44 AM, Ning Zhang <nzh...@fb.com<mailto:nzh...@fb.com>> wrote: Hi MIS, Thanks for the contribution! To allow broader audience to review, can you upload your patch to the JIRA and the review board (I can help you with the review board if it doesn't allow you to change the request). A couple of comments before uploading your patch: 1) the 5 sec keepAliveTime seems low. If the # of threads is more than the # of cores, does it mean the thread will be terminated after 5 secs after it is waiting to get scheduled? 2) do you need to call execService.shutDown() in case of a Throwable is caught? On Mar 3, 2011, at 10:09 AM, MIS wrote: Hi, Ning Just to be clear on what I was suggesting, I have created a patch only for this file. Please have a look. Thanks, MIS. On Thu, Mar 3, 2011 at 5:50 PM, M IS <misapa...@gmail.com<mailto:misapa...@gmail.com>> wrote: This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/460/ trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/UpdateInputAccessTimeHook.java<https://reviews.apache.org/r/460/diff/1/?file=13550#file13550line82> (Diff revision 1) public void run(SessionState sess, Set<ReadEntity> inputs, 77 Thread[] threads = new Thread[nThreads]; How about going for a centralized thread pool and submitting the tasks for that pool. This can have advantages like, we need not have to create threads and we could come to know of the status of the task submitted through the future object. And use this future to to wait till the task is finished. We can re factor the code to make UpdateWorker to implement Runnable instead of extending of Thread. - M On March 3rd, 2011, 12:53 a.m., Ning Zhang wrote: Review request for hive. By Ning Zhang. Updated 2011-03-03 00:53:49 Description define hive.hooks.parallel.degree to control max # of thread to update metastore in parallel. Diffs * trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (1076459) * trunk/conf/hive-default.xml (1076459) * trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (1076459) * trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/UpdateInputAccessTimeHook.java (1076459) View Diff<https://reviews.apache.org/r/460/diff/> <HIVE-2026_1.patch> <HIVE-2026_2.patch>
