You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Sergey Shelukhin <se...@hortonworks.com> on 2016/04/22 20:47:45 UTC
Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/
-----------------------------------------------------------
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 fabb8ab
llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java ce03de0
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java e99e689
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java 76ba225
llap-server/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 8c7a539
Diff: https://reviews.apache.org/r/46579/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On April 23, 2016, 12:38 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java, line 193
> > <https://reviews.apache.org/r/46579/diff/1/?file=1357677#file1357677line193>
> >
> > Can we restrict this configuration parameter to be used within HS2 only ?
> > Any other client (CLI) would have it off by default - and fetch the token from LLAP itself.
>
> Siddharth Seth wrote:
> Also. the config property name will likely need a rename if this is restricted to HS2.
changed to have both options
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review130230
-----------------------------------------------------------
On April 22, 2016, 6:47 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 6:47 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 fabb8ab
> llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java ce03de0
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java e99e689
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java 76ba225
> llap-server/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 8c7a539
>
> Diff: https://reviews.apache.org/r/46579/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Siddharth Seth <ss...@apache.org>.
> On April 23, 2016, 12:38 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java, line 193
> > <https://reviews.apache.org/r/46579/diff/1/?file=1357677#file1357677line193>
> >
> > Can we restrict this configuration parameter to be used within HS2 only ?
> > Any other client (CLI) would have it off by default - and fetch the token from LLAP itself.
Also. the config property name will likely need a rename if this is restricted to HS2.
- Siddharth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review130230
-----------------------------------------------------------
On April 22, 2016, 6:47 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 6:47 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 fabb8ab
> llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java ce03de0
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java e99e689
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java 76ba225
> llap-server/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 8c7a539
>
> Diff: https://reviews.apache.org/r/46579/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Siddharth Seth <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review130230
-----------------------------------------------------------
Mostly looks good. Left some comments - mostly minor issues. The one about the Configuration being used only in HS2 I think would be useful to fix.
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 73)
<https://reviews.apache.org/r/46579/#comment193938>
Minor: static class
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 156)
<https://reviews.apache.org/r/46579/#comment193939>
Minor: static class
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 166)
<https://reviews.apache.org/r/46579/#comment193940>
Either here, or when constructing the Impl - a log line indicating where the token is being fetched from will be useful - similar to the one in RemoteImpl.
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 168)
<https://reviews.apache.org/r/46579/#comment193941>
Can we restrict this configuration parameter to be used within HS2 only ?
Any other client (CLI) would have it off by default - and fetch the token from LLAP itself.
me
- Siddharth Seth
On April 22, 2016, 6:47 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 6:47 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 fabb8ab
> llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java ce03de0
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java e99e689
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java 76ba225
> llap-server/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 8c7a539
>
> Diff: https://reviews.apache.org/r/46579/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Siddharth Seth <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review130365
-----------------------------------------------------------
Ship it!
Looks good. One minor comment - in case you want to make a change before commit - "hs2" as a constant somewhere rather than referring the string literal separately in 2 places, and maybe call it 'hiveserver' in case there's ever an hs3 / hs2 goes away.
- Siddharth Seth
On April 23, 2016, 1:57 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated April 23, 2016, 1:57 a.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 926806b
> llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java ce03de0
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java e99e689
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java 76ba225
> llap-server/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 8c7a539
>
> Diff: https://reviews.apache.org/r/46579/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Lefty Leverenz <le...@gmail.com>.
> On May 6, 2016, midnight, Lefty Leverenz wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, lines 2694-2698
> > <https://reviews.apache.org/r/46579/diff/3/?file=1374107#file1374107line2694>
> >
> > My usual comment: please spell out the abbreviations ZK and HS2 (at least once each) for the benefit of newcomers. Neither one comes up near the top of a google search.
+1 Looks good, thanks.
- Lefty
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review131971
-----------------------------------------------------------
On May 17, 2016, 12:56 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 12:56 a.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 541af57
> llap-client/pom.xml 4a75bbb
> 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 5731b2c
> 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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Lefty Leverenz <le...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review131971
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2694 - 2698)
<https://reviews.apache.org/r/46579/#comment196012>
My usual comment: please spell out the abbreviations ZK and HS2 (at least once each) for the benefit of newcomers. Neither one comes up near the top of a google search.
- Lefty Leverenz
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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Siddharth Seth <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review133629
-----------------------------------------------------------
Ship it!
Looks good. Think HIVE-13698 becomes a blocker - from a performance perspective, as well as to make sure the threads started by the SecretManager are shutdown.
- Siddharth Seth
On May 17, 2016, 12:56 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 12:56 a.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 541af57
> llap-client/pom.xml 4a75bbb
> 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 5731b2c
> 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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Siddharth Seth <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review133646
-----------------------------------------------------------
Ship it!
Ship It!
- Siddharth Seth
On May 17, 2016, 9:58 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 9:58 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 541af57
> llap-client/pom.xml 4a75bbb
> 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 5731b2c
> 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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On May 17, 2016, 10:01 p.m., Siddharth Seth wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java, line 376
> > <https://reviews.apache.org/r/46579/diff/4-5/?file=1385106#file1385106line376>
> >
> > Required here as well ?
remote one doesn't start threads; also see the comment on the cache field
> On May 17, 2016, 10:01 p.m., Siddharth Seth wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java, line 347
> > <https://reviews.apache.org/r/46579/diff/4-5/?file=1385106#file1385106line347>
> >
> > null check required on the notification itself ?
not as far as I can tell. Evne the value check is just paranoid, it's probably unneeded
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review133639
-----------------------------------------------------------
On May 17, 2016, 9:58 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 9:58 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 541af57
> llap-client/pom.xml 4a75bbb
> 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 5731b2c
> 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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Siddharth Seth <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review133639
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 347)
<https://reviews.apache.org/r/46579/#comment198197>
null check required on the notification itself ?
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 376)
<https://reviews.apache.org/r/46579/#comment198195>
Required here as well ?
- Siddharth Seth
On May 17, 2016, 9:58 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46579/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 9:58 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 541af57
> llap-client/pom.xml 4a75bbb
> 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 5731b2c
> 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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/
-----------------------------------------------------------
(Updated May 17, 2016, 9:58 p.m.)
Review request for hive, Gunther Hagleitner, Siddharth Seth, and Vikram Dixit Kumaraswamy.
Repository: hive-git
Description
-------
see JIRA
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 541af57
llap-client/pom.xml 4a75bbb
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 5731b2c
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
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/
-----------------------------------------------------------
(Updated May 17, 2016, 12:56 a.m.)
Review request for hive, Gunther Hagleitner, Siddharth Seth, and Vikram Dixit Kumaraswamy.
Repository: hive-git
Description
-------
see JIRA
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 541af57
llap-client/pom.xml 4a75bbb
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 5731b2c
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
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> 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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Siddharth Seth <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/#review133466
-----------------------------------------------------------
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java (line 32)
<https://reviews.apache.org/r/46579/#comment197906>
Nit: Final
llap-common/src/java/org/apache/hadoop/hive/llap/DaemonId.java (line 24)
<https://reviews.apache.org/r/46579/#comment197907>
Can appId be removed from the parameter list. Doesn't look like it's used anywhere.
llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java (line 57)
<https://reviews.apache.org/r/46579/#comment197914>
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.
llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java (line 61)
<https://reviews.apache.org/r/46579/#comment197909>
Can this refer to a hardcoded string in the slider code base - very prone to breaking otherwise.
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 347)
<https://reviews.apache.org/r/46579/#comment197913>
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 ?
- Siddharth Seth
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
>
>
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 46579: HIVE-13449 LLAP: HS2 should get the token
directly, rather than from LLAP
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46579/
-----------------------------------------------------------
(Updated April 23, 2016, 1:57 a.m.)
Review request for hive, Gunther Hagleitner, Siddharth Seth, and Vikram Dixit Kumaraswamy.
Repository: hive-git
Description
-------
see JIRA
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 926806b
llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java ce03de0
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java e99e689
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java 76ba225
llap-server/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 8c7a539
Diff: https://reviews.apache.org/r/46579/diff/
Testing
-------
Thanks,
Sergey Shelukhin