Return-Path: X-Original-To: apmail-hive-dev-archive@www.apache.org Delivered-To: apmail-hive-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 099C818763 for ; Sat, 19 Dec 2015 02:00:30 +0000 (UTC) Received: (qmail 29036 invoked by uid 500); 19 Dec 2015 02:00:29 -0000 Delivered-To: apmail-hive-dev-archive@hive.apache.org Received: (qmail 28954 invoked by uid 500); 19 Dec 2015 02:00:29 -0000 Mailing-List: contact dev-help@hive.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hive.apache.org Delivered-To: mailing list dev@hive.apache.org Received: (qmail 28937 invoked by uid 99); 19 Dec 2015 02:00:29 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 19 Dec 2015 02:00:29 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id CD6F92954C2; Sat, 19 Dec 2015 02:00:28 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3507224508764635886==" MIME-Version: 1.0 Subject: Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread From: "Siddharth Seth" To: "Siddharth Seth" , "Gunther Hagleitner" Cc: "hive" , "Sergey Shelukhin" Date: Sat, 19 Dec 2015 02:00:28 -0000 Message-ID: <20151219020028.1672.39222@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Siddharth Seth" X-ReviewGroup: hive X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/41377/ X-Sender: "Siddharth Seth" References: <20151218221030.1672.8372@reviews.apache.org> In-Reply-To: <20151218221030.1672.8372@reviews.apache.org> Reply-To: "Siddharth Seth" X-ReviewRequest-Repository: hive-git --===============3507224508764635886== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41377/#review111342 ----------------------------------------------------------- TezSessionState - does that need to be synchronized on various methods like openSession, getSession, closeSession. To me, it seems like access to these variables is not thread safe. Even something like TezSessionState.queueName. That's setup in the main thread in TezSessionPoolManager.setupSessionPool. Then it is accessed in separate threads - without being a final field, or protected by synchronzied blocks. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 974) General: Some docs mentioning that this method is meant to work across threads and processes, and how that complicates the notifiers / sleep. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 987) Maybe a higher interval or #of attempts, so that most files in the same JVM don't go into the IOException path. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1025) Seems like this is chceked far too often. Required here. Wonder if it can be removed further up. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1027) Name of the file in the log will be useful ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1032) waitAttempts isn't actually used. Drop from the api, or use instead of the sleep for loop in the exception block. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 88) Why is this required ? and why not in the single thread case. ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java (line 118) Is there any part of the test which verifies that threads are being used / sessions are being setup in parallel. Not sure if there's a good way to verify that though - without setting up explicit properties for tests only. - Siddharth Seth On Dec. 18, 2015, 10:10 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41377/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2015, 10:10 p.m.) > > > Review request for hive, Gunther Hagleitner and Siddharth Seth. > > > Repository: hive-git > > > Description > ------- > > see JIRA > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 36e281a > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 6e196e6 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 0d84340 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 > ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 11c0325 > > Diff: https://reviews.apache.org/r/41377/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > > --===============3507224508764635886==--