> On May 17, 2016, 12:01 a.m., Siddharth Seth wrote: > > llap-common/src/java/org/apache/hadoop/hive/llap/DaemonId.java, line 24 > > <https://reviews.apache.org/r/46579/diff/3/?file=1374112#file1374112line24> > > > > Can appId be removed from the parameter list. Doesn't look like it's > > used anywhere.
it can be useful in future > On May 17, 2016, 12:01 a.m., Siddharth Seth wrote: > > llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java, line 57 > > <https://reviews.apache.org/r/46579/diff/3/?file=1374113#file1374113line57> > > > > Not used anywhere ? > > Also, this ends up being very flaky - with the "org-apache-slider" > > string hardcoded there. Referencing the actual slider constant would be > > better. > > Removing the unused code would be better though. commented it out for now > On May 17, 2016, 12:01 a.m., Siddharth Seth wrote: > > llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java, line 61 > > <https://reviews.apache.org/r/46579/diff/3/?file=1374113#file1374113line61> > > > > Can this refer to a hardcoded string in the slider code base - very > > prone to breaking otherwise. it's not accessible there; filed a slider jira. This method has been commented out > On May 17, 2016, 12:01 a.m., Siddharth Seth wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java, line > > 353 > > <https://reviews.apache.org/r/46579/diff/3/?file=1374121#file1374121line353> > > > > Is this supposed to be creating the LocalClient each time (whcih > > creates the secret manager), or is the intent to eventually share the > > SecretManager ? > > Similarly for the next line - create an instance of the factory each > > time, create client and then the token. The code explicitly mentions a new > > client being created for thread safety. Doesn't really matter if a new > > factory is created each time. > > > > Looks like not sharing the SecretManager could be expensive given the > > KDC login each time ? followed up in HIVE-13698 - Sergey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46579/#review133466 ----------------------------------------------------------- On May 5, 2016, 9:55 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46579/ > ----------------------------------------------------------- > > (Updated May 5, 2016, 9:55 p.m.) > > > Review request for hive, Gunther Hagleitner, Siddharth Seth, and Vikram Dixit > Kumaraswamy. > > > Repository: hive-git > > > Description > ------- > > see JIRA > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java bb74d99 > llap-client/pom.xml 50c06a4 > llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapProxy.java > 424769f > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClientFactory.java > PRE-CREATION > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java > PRE-CREATION > llap-common/src/java/org/apache/hadoop/hive/llap/DaemonId.java 18355e6 > llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java ce03de0 > > llap-common/src/java/org/apache/hadoop/hive/llap/impl/LlapManagementProtocolClientImpl.java > cd11bdb > > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java > edf9b18 > > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java > e662de9 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java > db8bfa6 > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java > f958bc4 > > llap-server/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java > c54e726 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java fd6465a > > Diff: https://reviews.apache.org/r/46579/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
