> On May 14, 2020, 4:31 a.m., Ashutosh Chauhan wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java > > Lines 646-651 (patched) > > <https://reviews.apache.org/r/72499/diff/1/?file=2231454#file2231454line647> > > > > Is this logic needed? You already have valueloader in get() which must > > return a ugi, so it cant be null. > > Rajesh Balamohan wrote: > Yes, value loader is for initial miss. This is to avoid single connection > becoming a contention for AM communication. > https://issues.apache.org/jira/browse/HIVE-16634 > > Ashutosh Chauhan wrote: > Not sure I follow. Can you add comments in code to explain the need for > this?
Addded comment in recent upload of the patch. Earlier patch wasn't uploaded correctly. Value loader would be returning the queue (not ugi directly). Queue can maintain set of connections to same AM. Depending on query pattern, we need multiple connections to AM (addressed in HIVE-16634). Actually we are retaining the same code here as it was in QueryInfo earlier. > On May 14, 2020, 4:31 a.m., Ashutosh Chauhan wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java > > Lines 663 (patched) > > <https://reviews.apache.org/r/72499/diff/1/?file=2231454#file2231454line664> > > > > if its null, then its programming error. Better to not do this null > > check and offer without checking for null. > > Ashutosh Chauhan wrote: > better to throw NPE then to leak ugi failing to return to pool. This is not a leak. It is gc-able ugi in case someone returns the ugi after expiry. - Rajesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72499/#review220748 ----------------------------------------------------------- On May 14, 2020, 7:14 a.m., Rajesh Balamohan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72499/ > ----------------------------------------------------------- > > (Updated May 14, 2020, 7:14 a.m.) > > > Review request for hive, Ashutosh Chauhan and Gopal V. > > > Bugs: HIVE-23446 > https://issues.apache.org/jira/browse/HIVE-23446 > > > Repository: hive-git > > > Description > ------- > > Currently UGI pool is maintained at QueryInfo level. However, when short > queries and lots of AMs are there, it ends missing IPC connection cache. Too > many connections are are also established. Patch tries to avoid that by > maintaining this at ContainerRunner level. It retains the current behaviour > of having multiple connection to same AM (otherwise can get bottlenecked on > single connection) > > > Diffs > ----- > > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java > 6a13b55e69 > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryInfo.java > 00fed15d2b > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java > eae8e08540 > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java > 50dec4759e > > > Diff: https://reviews.apache.org/r/72499/diff/3/ > > > Testing > ------- > > > Thanks, > > Rajesh Balamohan > >
