> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Line 88 (original), 100 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line103> > > > > nit: <=1 ?
there's a check on the previous line > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Line 98 (original), 110 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line113> > > > > would be cleaner with executor service + thread factory to name the > > thread based on idx or session name instead of using FutureTask directly. this makes special casing below difficult, and doesn't seem necessary since these are one-shot thread. Why create an executor service for that and make sure to close it. > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Line 102 (original), 114 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line117> > > > > why this special casing here? why are invoking threadTasks[0].run() > > only for i== 0 and not others ? Because this is blocking, so might as well use the current thread. It's the same as the old logic that has been moved. > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Lines 120 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line131> > > > > Better to use ListenableFuture instead of potential blocking here > > during future.get() We want to block here... old code > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Lines 130 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line146> > > > > use notEmpty.awaitUninterruptibly() ? won't wake up for spurious > > interrupts. no need to loop with that. Hmm... I don't like setting things to wait forever as I've seen it cause bugs many times in the past. With the loop, it rechecks the pool so it's harder to introduce bugs, although now it's strictly speaking unnecessary to recheck > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Lines 335 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line358> > > > > use sequence number from znode for now? assuming restart will generate > > new znode and sequence number. > > else UUID in service instance. > > > > Can you add logging here to record the create/remove/update events for > > debugging in case if this issue happens. > > > > Please file jira for tracking this. Filed; there's already logging there > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Lines 347 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line370> > > > > make this dummy true future a static constant. I am not sure it would be safe if multiple threads e.g. try to add listeners to it or otherwise try to work it into their workflow. > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Lines 372 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line395> > > > > this entire block looks repeated here and for initial sessions. put > > this in a separate method instead? also can use single thread pool/executor > > service for this as mentioned in earlier comments. The difference is that current thread is not used. Again executor is not needed because I want the threads to finish and go away. > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Lines 401 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line424> > > > > when, how, who will drain sessions in toClose? I don't see it being > > drained and closed anywhere in this patch. The caller. Added javadoc. syncWork.toDestroyNoRestart is passed as toClose by WM > On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > > Lines 434 (patched) > > <https://reviews.apache.org/r/63230/diff/2/?file=1867959#file1867959line457> > > > > use CountDownLatch instead? before creating CreateSessionsRunnable we > > know how many sessions we want to create (initial or delta). We can set > > that to latch and these runnables can simply decrement it after session > > creation. Main thread can wait for it to become zero. Easier to read and > > few LOC. We actually pass deltaRemaining here during resize, so that if the pool is resized down concurrently with async expansion we won't create and kill the sessions - Sergey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63230/#review189249 ----------------------------------------------------------- On Oct. 24, 2017, 11:58 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63230/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2017, 11:58 p.m.) > > > Review request for hive and Prasanth_J. > > > Repository: hive-git > > > Description > ------- > > see jira > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 0b4abb824f > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java > d978a25b14 > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java > 45c3e38dcc > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java > a326db3ab0 > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java > da93a3a791 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java > b67c933b19 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java > 9b4714f1d7 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java > 613522357e > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java > 144816862d > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java > 81d6b859a6 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java > d725e90475 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java > 209cf57a6a > ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java > 59efd43be6 > ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java > 258a865fef > > > Diff: https://reviews.apache.org/r/63230/diff/2/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
