----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47563/#review135706 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2880) <https://reviews.apache.org/r/47563/#comment200742> Is this needed ? Can we create the instance on first access instead of adding a config. llap-client/src/java/org/apache/hadoop/hive/llap/SubmitWorkInfo.java (line 46) <https://reviews.apache.org/r/47563/#comment200744> We should get rid of TaskSpec. Adding vertex parallelism to this allows for that. If TaskSpec is required in non-secure mode - it can become a union like the payload is. TaskSpec is otherwise a very heavyweight object. Once all the task number attempt number etc is set by the client - we can send VertexSpec only once - instead of for each Task (that would be a reasonably perf enhancement for a future jira) llap-client/src/java/org/apache/hadoop/hive/llap/SubmitWorkInfo.java (lines 146 - 147) <https://reviews.apache.org/r/47563/#comment200746> Nit: Rename to getVertexBinary ? llap-common/src/java/org/apache/hadoop/hive/llap/coordinator/LlapCoordinator.java (line 28) <https://reviews.apache.org/r/47563/#comment200748> Javadoc would be useful. (Seems to be in the impl isntead of here), especially given that LlapCooridnator is really easy to confuse for something that does fragment handling. This is more like a LlapSecurityCoordinator / LlapSecurityHelper at the moment. llap-ext-client/src/java/org/apache/hadoop/hive/llap/LlapBaseInputFormat.java (line 315) <https://reviews.apache.org/r/47563/#comment200749> Is a signature always expected ? If so, may as well convert this to a precondition. Otherwise, in what case will it not be expected ? llap-ext-client/src/java/org/apache/hadoop/hive/llap/LlapBaseInputFormat.java (line 324) <https://reviews.apache.org/r/47563/#comment200750> This can be replaced by 'taskNum' - that leaves only vertexParallelism from TaskSpec - which can be included in the Info object. llap-server/src/java/org/apache/hadoop/hive/llap/coordinator/LlapCoordinatorImpl.java (line 1) <https://reviews.apache.org/r/47563/#comment200751> Should this be in llap-server, or llap-common/llap-client. Don't think HS2 needs to depend on llap-server for anything. llap-server/src/java/org/apache/hadoop/hive/llap/coordinator/LlapCoordinatorImpl.java (line 138) <https://reviews.apache.org/r/47563/#comment200753> Catch Exception instead of Throwable ? Also log the message so that we know what's going on if this happens. llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java (line 40) <https://reviews.apache.org/r/47563/#comment200755> Sharing this with the main SecretManager will be in some other patch ? llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java (line 58) <https://reviews.apache.org/r/47563/#comment200757> Need to look at what this 'user' is used for. I'm not sure why the field is actually needed. llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java (line 74) <https://reviews.apache.org/r/47563/#comment200752> Log on close ? At INFO at least initially, later at debug ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 353) <https://reviews.apache.org/r/47563/#comment200758> Not sure why the user is required in the client. Seems like this will cause all kinds of issues with doAs=true. Multiple llap instances is not supported yet - can avoid adding bits and pieces towards it. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java (line 328) <https://reviews.apache.org/r/47563/#comment200760> Null check ? ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java (line 339) <https://reviews.apache.org/r/47563/#comment200762> Don't think this is enforced at the moment. Is it required. Can we use the SignedVertexSpec with and without security to have a consistent code path ? ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java (line 389) <https://reviews.apache.org/r/47563/#comment200763> Nit: rename to createSignedVertexSpec ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java (line 392) <https://reviews.apache.org/r/47563/#comment200764> This seems like a guaranteed NPE. The user will be set in a proto - which will throw a NullPointerException. Hopefully this will be caught by some tests. Would be good to add a couple of tests here as well. - Siddharth Seth On May 27, 2016, 9:32 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47563/ > ----------------------------------------------------------- > > (Updated May 27, 2016, 9:32 p.m.) > > > Review request for hive, Jason Dere and Siddharth Seth. > > > Repository: hive-git > > > Description > ------- > > see JIRA. Please ignore the first iteration, RB doesn't allow base patches on > submit > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6a404bd > llap-client/src/java/org/apache/hadoop/hive/llap/SubmitWorkInfo.java > 6704294 > llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapProxy.java > 6c2618b > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java > f10351b > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClientImpl.java > PRE-CREATION > > llap-common/src/java/org/apache/hadoop/hive/llap/coordinator/LlapCoordinator.java > PRE-CREATION > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java > PRE-CREATION > > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java > PRE-CREATION > > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java > 465b204 > > llap-common/src/java/org/apache/hadoop/hive/llap/security/SigningSecretManager.java > PRE-CREATION > llap-common/src/java/org/apache/hadoop/hive/llap/tez/Converters.java > e43b72b > llap-common/src/test/org/apache/hadoop/hive/llap/tez/TestConverters.java > 349ee14 > > llap-ext-client/src/java/org/apache/hadoop/hive/llap/LlapBaseInputFormat.java > 4306c22 > > llap-server/src/java/org/apache/hadoop/hive/llap/coordinator/LlapCoordinatorImpl.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java > 2524dc2 > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java > PRE-CREATION > > llap-server/src/test/org/apache/hadoop/hive/llap/security/TestLlapSignerImpl.java > PRE-CREATION > > llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskCommunicator.java > 026df3b > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java > 83d492a > service/src/java/org/apache/hive/service/server/HiveServer2.java d61edf5 > > Diff: https://reviews.apache.org/r/47563/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
