----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40315/#review107706 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2348) <https://reviews.apache.org/r/40315/#comment166963> Description needs fixing. common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2351) <https://reviews.apache.org/r/40315/#comment166964> Don't think the default value - "*" - has any significance here. Replace by null - to avoid confusion. llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapIoProxy.java (line 76) <https://reviews.apache.org/r/40315/#comment166965> Rename class to LLAPProxy ? It's no longer limited to IO only. llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java (line 44) <https://reviews.apache.org/r/40315/#comment166966> This and readFields - eventually should be implemented using a Protobuf payload. Allows the token to evolve during rolling upgrades. Separate jira. llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java (line 71) <https://reviews.apache.org/r/40315/#comment166967> Does a renewer for a token type have to be specified ? llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolClientImpl.java (line 128) <https://reviews.apache.org/r/40315/#comment166968> This could be moved into it's own protocol (but listening on the same server). The methods so far are for access from the AM. getTokens is to be used by Clients. What that also allows is for the annotations to change. getTokens() - protected by Kerberos, and cannot be obtained using a token. Remaining methods - require a token. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolClientImpl.java (line 129) <https://reviews.apache.org/r/40315/#comment166969> Rename to getDelegationToken ? llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java (line 79) <https://reviews.apache.org/r/40315/#comment166970> throws IOException not required. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java (line 133) <https://reviews.apache.org/r/40315/#comment166971> Sanity checks for the values. Empty strings are not allowed. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java (line 157) <https://reviews.apache.org/r/40315/#comment166972> Avoid using the ZK property names. Instead, the properties defined for LLAP should be used. (ZK properties could leak in from some other service which uses the same SecretManager) llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java (line 164) <https://reviews.apache.org/r/40315/#comment166973> New instances of Configuration if doing a set (this config is shared by several services) llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java (line 250) <https://reviews.apache.org/r/40315/#comment166974> YARN can take care of renewing delegation tokens - assuming the service supports it (i.e. the ZKSecretManager on one of the LLAP instances or a direct connection to ZK from the RM - but that isn't a good idea). Eventually, I believe the renweer would need to change to the RM service user. llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java (line 26) <https://reviews.apache.org/r/40315/#comment166975> How is the default value picked up ? (definitely not from the hive conf) OR What is the default value - "*" or " ". I'm not sure how other services handle this - but this can be set to " " by default on secure clusters, and "*" on non-secure clusters. llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java (line 32) <https://reviews.apache.org/r/40315/#comment166976> clone not required. llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 53) <https://reviews.apache.org/r/40315/#comment166977> This would matter when running under HiveServer ? or is the synchronization in LlapIoProxy taking care of this ? llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 59) <https://reviews.apache.org/r/40315/#comment166978> final. Also retryPolicy. llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 67) <https://reviews.apache.org/r/40315/#comment166979> make configurable ? llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 94) <https://reviews.apache.org/r/40315/#comment166980> Lots of TODOs - fix / convert to jira with a reference to the jira number ? llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 96) <https://reviews.apache.org/r/40315/#comment166981> Stop logging the token. llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 100) <https://reviews.apache.org/r/40315/#comment166983> Can be accessed via TaskCommunicatorContext. More on this in SessionState. llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 105) <https://reviews.apache.org/r/40315/#comment166982> Stop logging the token. llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 511) <https://reviews.apache.org/r/40315/#comment166984> Required for each host separately ? Setting the host may not be required. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 319) <https://reviews.apache.org/r/40315/#comment166985> Emailing the token to the AM in the Configuration is very avoidable. The token can be provided to Tez while creating the TezClient. TezClient will make this available to the TaskCommunicator via the TaskCommunicatorContext.getCredentials(). See TokenCache.get/setSessionToken. The static string defined in HiveConf to send this token could be shortened and moved out of HiveConf. Haven't looked at the details of the ZKSecretManager - but it looks like the Tokens issued by any of the LLAP instances can be used by an application to communicate with all other instances. Also, are the tokens the same for different applications ? - Siddharth Seth On Nov. 16, 2015, 7:45 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40315/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2015, 7:45 p.m.) > > > Review request for hive, Gopal V and Siddharth Seth. > > > Repository: hive-git > > > Description > ------- > > see JIRA > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 838f25c > llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapIoProxy.java > 4c31e32 > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java > PRE-CREATION > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/LlapDaemonProtocolBlockingPB.java > 5ad2344 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java > 98b1ccd > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolClientImpl.java > 4b13277 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java > 784c631 > > llap-server/src/java/org/apache/hadoop/hive/llap/protocol/LlapTaskUmbilicalProtocol.java > fae7654 > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapServerSecurityInfo.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapTokenSelector.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskCommunicator.java > d327fc0 > > llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapUmbilicalPolicyProvider.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java > 33e998c > > llap-server/src/main/resources/META-INF/services/org.apache.hadoop.security.SecurityInfo > PRE-CREATION > llap-server/src/protobuf/LlapDaemonProtocol.proto 0ba6acf > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapDaemonProtocolServerImpl.java > 8d45c95 > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 9ab3e98 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 07f26be > serde/src/java/org/apache/hadoop/hive/serde2/AbstractSerDe.java 9269ff4 > > Diff: https://reviews.apache.org/r/40315/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
