Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-31 Thread Siddharth Seth 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review135722
---


Ship it!




Ship It!

- Siddharth Seth


On May 28, 2016, 2:04 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated May 28, 2016, 2:04 a.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6a404bd 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  cffa493 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 5ab7b3c 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> fea3dc7 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapUgiFactoryFactory.java
>  PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  e0f0676 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
> ef2b7f7 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
> 4a96355 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-27 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/
---

(Updated May 28, 2016, 2:04 a.m.)


Review request for hive and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6a404bd 
  
llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
 cffa493 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 2524dc2 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
5ab7b3c 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 74359fa 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
fea3dc7 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
 b3b571d 
  
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapUgiFactoryFactory.java
 PRE-CREATION 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 e0f0676 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 a250882 
  shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
ef2b7f7 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
4a96355 

Diff: https://reviews.apache.org/r/46754/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-23 Thread Siddharth Seth 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review134432
---



Any possibility of simple unit tests ?


llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapUgiFactoryFactory.java
 (line 36)


Lets skip the reflection, and create a single UGI for now.

A follow up jira to create a UGI pool should take care of the potential 
single UGI perf issue.


- Siddharth Seth


On May 20, 2016, 1:49 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated May 20, 2016, 1:49 a.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9cc8fbe 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  cffa493 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> de817e3 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> fea3dc7 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapUgiFactoryFactory.java
>  PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  279baf1 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-19 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/
---

(Updated May 20, 2016, 1:49 a.m.)


Review request for hive and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9cc8fbe 
  
llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
 cffa493 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 2524dc2 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
de817e3 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 74359fa 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
fea3dc7 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
 b3b571d 
  
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapUgiFactoryFactory.java
 PRE-CREATION 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 279baf1 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 a250882 

Diff: https://reviews.apache.org/r/46754/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-03 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/
---

(Updated May 3, 2016, 9:07 p.m.)


Review request for hive and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 06a6906 
  
llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
 6981061 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 78b37f7 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
d23a44a 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 fcfa940 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
fea3dc7 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
 b3b571d 
  
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
 f958bc4 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 c6ba14e 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 08ee769 

Diff: https://reviews.apache.org/r/46754/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-03 Thread Sergey Shelukhin 


> On May 3, 2016, 3:14 p.m., Siddharth Seth wrote:
> > Looks good in terms of functionality. (Ship it if you think the reflection 
> > is not super brittle :))
> > I think we should get rid of the reflection to access private methods ASAP 
> > though - it can be really brittle and cause difficult to debug failures 
> > with different hadoop versions - ideally before a 2.1 release.
> > 1) The perf issue may not be valid, and we could get away with using a 
> > single UGI.
> > 2) A UGI pool.
> > If this gets committed as is, could you please open a follow up blocker 
> > ticket for 2.1 to remove the reflection. I can take that up at a later 
> > point.

We'd only be able to remove reflection in newer versions of Hadoop, so it will 
still remain in the shim, at least.
There's unfortunately no way to clone the UGI (that I see).
The problem with pools or a single UGI is that we cannot add credentials. 
Therefore, LLAP won't work against anything other than HDFS (e.g. HBase, etc.) 
in this mode... Maybe there should be a config flag for the case of bugs. 
However, I think UGI state is fairly stable, esp. the ctor and the getters 
probably won't change. We should decide, cause if we don't want reflection 
there's no point in committing this for now.


- Sergey


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review131507
---


On May 2, 2016, 9:44 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated May 2, 2016, 9:44 p.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 2814353 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  6981061 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  3d45c7a 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 63cb16b 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  fcfa940 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> fea3dc7 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
>  76ba225 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  24f4442 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  08ee769 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-03 Thread Siddharth Seth 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review131507
---


Fix it, then Ship it!




Looks good in terms of functionality. (Ship it if you think the reflection is 
not super brittle :))
I think we should get rid of the reflection to access private methods ASAP 
though - it can be really brittle and cause difficult to debug failures with 
different hadoop versions - ideally before a 2.1 release.
1) The perf issue may not be valid, and we could get away with using a single 
UGI.
2) A UGI pool.
If this gets committed as is, could you please open a follow up blocker ticket 
for 2.1 to remove the reflection. I can take that up at a later point.


llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
 (line 95)


Wondering if this ends up being a little too brittle. HADOOP-13081 itself 
may break this code.


- Siddharth Seth


On May 2, 2016, 9:44 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated May 2, 2016, 9:44 p.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 2814353 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  6981061 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  3d45c7a 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 63cb16b 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  fcfa940 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> fea3dc7 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
>  76ba225 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  24f4442 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  08ee769 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-02 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review131384
---




ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 


bogus


- Sergey Shelukhin


On May 2, 2016, 9:44 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated May 2, 2016, 9:44 p.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 2814353 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  6981061 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  3d45c7a 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 63cb16b 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  fcfa940 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> fea3dc7 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
>  76ba225 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  24f4442 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  08ee769 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-02 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/
---

(Updated May 2, 2016, 9:44 p.m.)


Review request for hive and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 2814353 
  
llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
 6981061 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 3d45c7a 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
63cb16b 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 fcfa940 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
fea3dc7 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
 b3b571d 
  
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
 76ba225 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 24f4442 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 08ee769 

Diff: https://reviews.apache.org/r/46754/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-05-02 Thread Sergey Shelukhin 


> On April 30, 2016, 7:06 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java,
> >  line 192
> > 
> >
> > If we're using a common UGI across all tasks - which is the kerberos 
> > UGI - I don't think we should add these credentials. That'll end up leaking 
> > credentials across tasks - and in general is not required assuming SQL 
> > standard auth.
> > For non-sql standard auth - we could continue with credentials only.

What about HBase and other systems? using the keytab would mean giving that 
user access or not having it at all. I'll add the clone of UGI...


- Sergey


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review131247
---


On April 27, 2016, 11:01 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated April 27, 2016, 11:01 p.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 5360ed4 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/ServiceInstanceSet.java
>  73f94f3 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  6af30d4 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  e80fb15 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 33b41e8 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  e99e689 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  2a60123 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> 6a72b4c 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> 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 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  24f4442 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  08ee769 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 8aca779 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/MapRecordProcessor.java 
> 0584ad8 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-04-30 Thread Siddharth Seth 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review131247
---




llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 (line 192)


If we're using a common UGI across all tasks - which is the kerberos UGI - 
I don't think we should add these credentials. That'll end up leaking 
credentials across tasks - and in general is not required assuming SQL standard 
auth.
For non-sql standard auth - we could continue with credentials only.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 (line 255)


This is the FileSyste.closeAllForUgi which will have to be handled.



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
 (line 122)


Why is all of this synchronization and class level statics required ? Will 
the principal / keytab path change or a running daemon.
Otherwise this seems to be an unnecessary lock across running fragments.

Think it'll be simpler to create a single UGI at startup for task 
execution, and then share that across tasks.
Alternately, if a single FS instance is a problem - create a pool of UGI 
instances - 1 per executor, and share those.

The allowCache, static lock, compare principal seems unnecessary.


- Siddharth Seth


On April 27, 2016, 11:01 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated April 27, 2016, 11:01 p.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 5360ed4 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/ServiceInstanceSet.java
>  73f94f3 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  6af30d4 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  e80fb15 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 33b41e8 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  e99e689 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  2a60123 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> 6a72b4c 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> 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 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  24f4442 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  08ee769 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 8aca779 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/MapRecordProcessor.java 
> 0584ad8 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46754: HIVE-13391 add an option to LLAP to use keytab to authenticate to read data

2016-04-27 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46754/#review130862
---




ql/src/java/org/apache/hadoop/hive/ql/exec/tez/MapRecordProcessor.java (line 61)


unneeded


- Sergey Shelukhin


On April 27, 2016, 11:01 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46754/
> ---
> 
> (Updated April 27, 2016, 11:01 p.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/UgiFactory.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 5360ed4 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/ServiceInstanceSet.java
>  73f94f3 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
>  6af30d4 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  e80fb15 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 33b41e8 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  e99e689 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  2a60123 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
> 6a72b4c 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/ColumnVectorProducer.java
>  b3b571d 
>   
> 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 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  24f4442 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  08ee769 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 8aca779 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/MapRecordProcessor.java 
> 0584ad8 
> 
> Diff: https://reviews.apache.org/r/46754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>