Re: Review Request 52795: SENTRY-1499: Add feature flag for using NotificationLog

2016-12-19 Thread Hao Hao 


> On Dec. 17, 2016, 1:48 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 147
> > 
> >
> > If some plugin fails, is there a need to undo any work?

The sync operation is meant to happen here(addPath). If plugin fails, then sync 
failed. I cannot see any work needs undo.


- Hao


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


On Dec. 16, 2016, 10:03 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52795/
> ---
> 
> (Updated Dec. 16, 2016, 10:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, kalyan kumar 
> kalvagadda, Li Li, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1499: Add feature flag for using NotificationLog
> 
> 1. Keep two SentryMetastorePostEventListeners. One for using notificationlog, 
> another for not.
> 2. Add feature flag for using HMSFollower and 
> SentryMetastorePostEventListeners with notification log.
> 
> Change-Id: I46a11d27ae0159a735b6049e9b7c78216d0e5346
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
>  75190c1805162e11baf7c4553668693e4e6bb3c4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerNotificationLog.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  a9d49f86961f1ced1af40afcfe6172f884a23d44 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  d80fa1e71c165b7f1faf1c50c451e049d76b770b 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  61b24fa0e9f221b75ae343fc534b4d82fbce272a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerInBuiltDeserializer.java
>  c4be62db71e67099f22f3ba8991e03a32fb0f28a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java
>  6f1886f942ca80ab4c356b81777d3ac336017292 
> 
> Diff: https://reviews.apache.org/r/52795/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52795: SENTRY-1499: Add feature flag for using NotificationLog

2016-12-19 Thread Hao Hao 

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

(Updated Dec. 20, 2016, 12:21 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, kalyan kumar 
kalvagadda, Li Li, and Sravya Tirukkovalur.


Repository: sentry


Description (updated)
---

SENTRY-1499: Add feature flag for using NotificationLog

1. Keep two SentryMetastorePostEventListeners. One for using notificationlog, 
another for not(Used the original SentryMetastorePostEventListeners from master 
https://github.com/apache/sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java).
2. Add feature flag for using HMSFollower and SentryMetastorePostEventListeners 
with notification log.

Change-Id: I46a11d27ae0159a735b6049e9b7c78216d0e5346


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 75190c1805162e11baf7c4553668693e4e6bb3c4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerNotificationLog.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 a9d49f86961f1ced1af40afcfe6172f884a23d44 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 d80fa1e71c165b7f1faf1c50c451e049d76b770b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 61b24fa0e9f221b75ae343fc534b4d82fbce272a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerInBuiltDeserializer.java
 c4be62db71e67099f22f3ba8991e03a32fb0f28a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java
 6f1886f942ca80ab4c356b81777d3ac336017292 

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


Testing
---


Thanks,

Hao Hao

Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

2016-12-19 Thread kalyan kumar kalvagadda 

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

(Updated Dec. 20, 2016, 12:07 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and 
Vadim Spector.


Changes
---

Optimized previous patch.


Repository: sentry


Description
---

SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

I have made changes assuming that grant option is either true/false removing 
unset. 
Also, added code so that sentry server could validate the TSentryPrivilege 
object constructed from the Thrift message received client. If the validation 
is failed exception is raised and appropriate error is message is sent.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 898632d 

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


Testing
---

Verfied the changes using sentry thrift client.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 52795: SENTRY-1499: Add feature flag for using NotificationLog

2016-12-19 Thread Hao Hao 


> On Dec. 17, 2016, 1:48 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 108
> > 
> >
> > Consider using static import for CLASS_SPLITTER

What is the reason to use static import here since CLASS_SPLITTER is only used 
once?


> On Dec. 17, 2016, 1:48 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 136
> > 
> >
> > This code is repeated everywhere - is it possible to handle it in a 
> > generic way?

Yeah, I agree. But I would rather to touch the core code in a follow up jira as 
the main purpose of this one is to add feature flag. I just simply copied back 
the original SentryMetastorePostEventListener class here.


> On Dec. 17, 2016, 1:48 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 151
> > 
> >
> > What is ment by drop here? The method is about creating a table.

AFAIU, the drop here was used to make sure no privilege existed before creating 
this table.


- Hao


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


On Dec. 16, 2016, 10:03 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52795/
> ---
> 
> (Updated Dec. 16, 2016, 10:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, kalyan kumar 
> kalvagadda, Li Li, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1499: Add feature flag for using NotificationLog
> 
> 1. Keep two SentryMetastorePostEventListeners. One for using notificationlog, 
> another for not.
> 2. Add feature flag for using HMSFollower and 
> SentryMetastorePostEventListeners with notification log.
> 
> Change-Id: I46a11d27ae0159a735b6049e9b7c78216d0e5346
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
>  75190c1805162e11baf7c4553668693e4e6bb3c4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerNotificationLog.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  a9d49f86961f1ced1af40afcfe6172f884a23d44 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  d80fa1e71c165b7f1faf1c50c451e049d76b770b 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  61b24fa0e9f221b75ae343fc534b4d82fbce272a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerInBuiltDeserializer.java
>  c4be62db71e67099f22f3ba8991e03a32fb0f28a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java
>  6f1886f942ca80ab4c356b81777d3ac336017292 
> 
> Diff: https://reviews.apache.org/r/52795/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

2016-12-19 Thread kalyan kumar kalvagadda 

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

(Updated Dec. 19, 2016, 11:25 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and 
Vadim Spector.


Changes
---

I'm working on SENTRY-1547 and SENTRY-1548. Fixes for both of them are in 
place. Last patch was bit confusing as weel as I had to remove the changes done 
for SENTRY-1547. 
This diff has changes for both of them.


Repository: sentry


Description
---

SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

I have made changes assuming that grant option is either true/false removing 
unset. 
Also, added code so that sentry server could validate the TSentryPrivilege 
object constructed from the Thrift message received client. If the validation 
is failed exception is raised and appropriate error is message is sent.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 898632d 

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


Testing
---

Verfied the changes using sentry thrift client.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-19 Thread Vamsee Yarlagadda 


> On Dec. 19, 2016, 8:15 p.m., Hao Hao wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 1720
> > 
> >
> > It looks like fromNULLCol is handling null string representation 
> > "__NULL__" as well. If we remove processing of such string, we need to 
> > handle it when processing TSentryPriviege? Fail to do so, may bring API 
> > backward incompatibility.

Good catch. I will see what i can do here.


- Vamsee


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


On Dec. 16, 2016, 9:17 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 16, 2016, 9:17 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  868e67720196f443cd281ec4e80ad552bf86e569 
> 
> Diff: https://reviews.apache.org/r/54525/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test 
> -Dtest=TestSentryStore -DfailIfNoTests=false 
> ...
> ---
>  T E S T S
> ---
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from 
> SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - 
> in org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 
> Results :
> 
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] 
> 
> ```
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-19 Thread Hao Hao 

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 1720)


It looks like fromNULLCol is handling null string representation "__NULL__" 
as well. If we remove processing of such string, we need to handle it when 
processing TSentryPriviege? Fail to do so, may bring API backward 
incompatibility.


- Hao Hao


On Dec. 16, 2016, 9:17 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 16, 2016, 9:17 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  868e67720196f443cd281ec4e80ad552bf86e569 
> 
> Diff: https://reviews.apache.org/r/54525/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test 
> -Dtest=TestSentryStore -DfailIfNoTests=false 
> ...
> ---
>  T E S T S
> ---
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from 
> SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - 
> in org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 
> Results :
> 
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] 
> 
> ```
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>