Re: Review Request: HIVE-1988

2011-04-05 Thread Amareshwari Sriramadasu 

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

Ship it!


+1

- Amareshwari


On 2011-04-05 21:24:34, Devaraj Das wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/528/
> ---
> 
> (Updated 2011-04-05 21:24:34)
> 
> 
> Review request for hive.
> 
> 
> Summary
> ---
> 
> Fixes to some security issues discussed in HIVE-1988
> 
> 
> This addresses bug HIVE-1988.
> https://issues.apache.org/jira/browse/HIVE-1988
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift 
> 1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/DelegationTokenSecretManager.java
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java
>  1089155 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java
>  1089155 
> 
> Diff: https://reviews.apache.org/r/528/diff
> 
> 
> Testing
> ---
> 
> New unit test added and that passes. All unit tests passed.
> 
> 
> Thanks,
> 
> Devaraj
> 
>

Re: Review Request: HIVE-1988

2011-04-05 Thread Devaraj Das 

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

(Updated 2011-04-05 21:24:34.129643)


Review request for hive.


Changes
---

Addressed Amareshwari's comments.


Summary
---

Fixes to some security issues discussed in HIVE-1988


This addresses bug HIVE-1988.
https://issues.apache.org/jira/browse/HIVE-1988


Diffs (updated)
-

  http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift 
1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/DelegationTokenSecretManager.java
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java
 1089155 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java
 1089155 

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


Testing
---

New unit test added and that passes. All unit tests passed.


Thanks,

Devaraj

Re: Review Request: HIVE-1988

2011-04-05 Thread Devaraj Das 


> On 2011-04-05 07:52:15, Amareshwari Sriramadasu wrote:
> > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
> >  line 152
> > 
> >
> > HadoopShims.isSecureShimImpl() is not called anywhere else. Shall we 
> > remove it if not required anymore?

I suggest we leave it there. This seems like a useful method, and I am actually 
using it in another patch.


> On 2011-04-05 07:52:15, Amareshwari Sriramadasu wrote:
> > http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java,
> >  lines 144-156
> > 
> >
> > Do you want to move this into setup(), as it is common in both 
> > testcases?

Done


> On 2011-04-05 07:52:15, Amareshwari Sriramadasu wrote:
> > http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java,
> >  lines 192-209
> > 
> >
> > code looks duplicated. Can it be refactored by passing group names to a 
> > method?

Done


- Devaraj


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


On 2011-03-29 10:26:38, Devaraj Das wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/528/
> ---
> 
> (Updated 2011-03-29 10:26:38)
> 
> 
> Review request for hive.
> 
> 
> Summary
> ---
> 
> Fixes to some security issues discussed in HIVE-1988
> 
> 
> This addresses bug HIVE-1988.
> https://issues.apache.org/jira/browse/HIVE-1988
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift 
> 1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/DelegationTokenSecretManager.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java
>  1085623 
> 
> Diff: https://reviews.apache.org/r/528/diff
> 
> 
> Testing
> ---
> 
> New unit test added and that passes. All unit tests passed.
> 
> 
> Thanks,
> 
> Devaraj
> 
>

Re: Review Request: HIVE-1988

2011-04-05 Thread Amareshwari Sriramadasu 

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



http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java


HadoopShims.isSecureShimImpl() is not called anywhere else. Shall we remove 
it if not required anymore?



http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java


Do you want to move this into setup(), as it is common in both testcases?



http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java


code looks duplicated. Can it be refactored by passing group names to a 
method?


- Amareshwari


On 2011-03-29 10:26:38, Devaraj Das wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/528/
> ---
> 
> (Updated 2011-03-29 10:26:38)
> 
> 
> Review request for hive.
> 
> 
> Summary
> ---
> 
> Fixes to some security issues discussed in HIVE-1988
> 
> 
> This addresses bug HIVE-1988.
> https://issues.apache.org/jira/browse/HIVE-1988
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift 
> 1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/DelegationTokenSecretManager.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java
>  1085623 
>   
> http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java
>  1085623 
> 
> Diff: https://reviews.apache.org/r/528/diff
> 
> 
> Testing
> ---
> 
> New unit test added and that passes. All unit tests passed.
> 
> 
> Thanks,
> 
> Devaraj
> 
>

Review Request: HIVE-1988

2011-03-29 Thread Devaraj Das 

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

Review request for hive.


Summary
---

Fixes to some security issues discussed in HIVE-1988


This addresses bug HIVE-1988.
https://issues.apache.org/jira/browse/HIVE-1988


Diffs
-

  http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift 
1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/DelegationTokenSecretManager.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java
 1085623 

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


Testing
---

New unit test added and that passes.


Thanks,

Devaraj

Re: Review Request: HIVE-1988

2011-03-29 Thread Devaraj Das 

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

(Updated 2011-03-29 10:26:38.374832)


Review request for hive.


Summary
---

Fixes to some security issues discussed in HIVE-1988


This addresses bug HIVE-1988.
https://issues.apache.org/jira/browse/HIVE-1988


Diffs
-

  http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift 
1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/DelegationTokenSecretManager.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java
 1085623 
  
http://svn.apache.org/repos/asf/hive/trunk/shims/src/test/org/apache/hadoop/hive/thrift/TestHadoop20SAuthBridge.java
 1085623 

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


Testing (updated)
---

New unit test added and that passes. All unit tests passed.


Thanks,

Devaraj