----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63890/#review191387 -----------------------------------------------------------
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestTriggersNoTezSessionPool.java Line 59 (original), 58 (patched) <https://reviews.apache.org/r/63890/#comment269182> doesn't look like pool is used ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java Line 716 (original), 717 (patched) <https://reviews.apache.org/r/63890/#comment269183> nit: WorkloadManager.getInstance is enough to determine if it's enabled. WM is initialized based on queue name only ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java Lines 743 (patched) <https://reviews.apache.org/r/63890/#comment269184> hmm... why is this an either or case? it's possible to have both in the same HS2. Should it call the else in both cases? ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java Line 511 (original), 511 (patched) <https://reviews.apache.org/r/63890/#comment269186> openSessions appears to be synchronized elsewhere, accessing it here may not be thread safe. Might make sense to call this under the same sync block that makes the changes to the list. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java Lines 517 (patched) <https://reviews.apache.org/r/63890/#comment269185> this seems to get all triggers for the pools... I can file a follow-up jira, I think we need to make addition of triggers to Tez more explicit service/src/java/org/apache/hive/service/server/HiveServer2.java Line 218 (original), 220 (patched) <https://reviews.apache.org/r/63890/#comment269187> same about else standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/WMFullResourcePlan.java Line 329 (original), 329 (patched) <https://reviews.apache.org/r/63890/#comment269189> this looks like a change to generated code without a thrift change. The null check (or isSet check?) should be in the caller - Sergey Shelukhin On Nov. 17, 2017, 2:20 a.m., Prasanth_J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63890/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2017, 2:20 a.m.) > > > Review request for hive and Sergey Shelukhin. > > > Bugs: HIVE-18025 > https://issues.apache.org/jira/browse/HIVE-18025 > > > Repository: hive-git > > > Description > ------- > > HIVE-18025: Push resource plan changes to tez/unmanaged sessions > > > Diffs > ----- > > > itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestTriggersNoTezSessionPool.java > bcce3dc > > itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestTriggersTezSessionPoolManager.java > b377275 > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java e7af5e0 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java > 4e48f15 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java > 769b24a > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 3990f95 > > ql/src/java/org/apache/hadoop/hive/ql/wm/MetastoreGlobalTriggersFetcher.java > 87c007f > service/src/java/org/apache/hive/service/server/HiveServer2.java 5a6d0c8 > > standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/WMFullResourcePlan.java > 1d21908 > > > Diff: https://reviews.apache.org/r/63890/diff/2/ > > > Testing > ------- > > > Thanks, > > Prasanth_J > >
