Sentry-jdk-1.7-v2 - Build # 53 - Fixed

[Apache Jenkins Server]

The Apache Jenkins build system has built Sentry-jdk-1.7-v2 (build #53)

Status: Fixed

Check console output at https://builds.apache.org/job/Sentry-jdk-1.7-v2/53/ to 
view the results.

Sentry-jdk-1.7 - Build # 752 - Failure

[Apache Jenkins Server]

The Apache Jenkins build system has built Sentry-jdk-1.7 (build #752)

Status: Failure

Check console output at https://builds.apache.org/job/Sentry-jdk-1.7/752/ to 
view the results.

Re: Review Request 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

[Hao Hao]

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

(Updated Jan. 10, 2017, 2:31 a.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
Vamsee Yarlagadda.


Repository: sentry


Description
---

SENTRY-1536: Refactor SentryStore transaction management to allow for extra 
transactions for a single permission update

Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347

To persist single permission change, it needs to combine multiple things in a 
single transaction:
1. Doing the actual operation (priv change)
2. Updating notification ID.

All the above steps are put into in a single transaction to guarantee that 
notificationID handling is atomic.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
 98349232bc658c39791e58b64949ecb975fff7a0 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 24729282bf17960152f87b1d3124caeafb47e6b2 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 3695709e03e683afe6196def53883e37e4910a1c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java
 2ff715f66ea6c2589a281b988438526546af3d3b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
 e678b575f86cd4797ad01f12e4a60fbeec9f84f5 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 8789a48c47e439bc2791cfe5a3b716b586025a7a 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 caa3c58b6d2e5874bea52379b9dd549a76698b9b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 f3cefd6a232bfb91db28f04bebcc98ab3c1ca658 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 a35c8d7dde485cf46d61968a211d1dbb6d9d6076 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
 1c3a4f29984379f5246da8d85fe661320c8a1043 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
 d601b1e107ab3c3a4d9cc5e3038a11547182c5c9 

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


Testing
---

New unit tests added in TestHMSFollower.


Thanks,

Hao Hao

Re: Review Request 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

[Hao Hao]

> On Jan. 6, 2017, 9:41 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  line 453
> > 
> >
> > SentrtPolicyStoreProcessor while handling such request, does call 
> > authorize function to make sure that user who is performing this is 
> > authorized to do perform it.
> > 
> > Should we be doing similar check here?

SentrtPolicyStoreProcessor is handling client side request(which could come 
from anyone), that is why they are performing checking. Here, it is when each 
time a create table event, or etc, we need to make sure any privilege relates 
to this object needs to be deleted. Thus, no need to check it.


- Hao


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


On Jan. 6, 2017, 1:37 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55246/
> ---
> 
> (Updated Jan. 6, 2017, 1:37 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1536: Refactor SentryStore transaction management to allow for extra 
> transactions for a single permission update
> 
> Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347
> 
> To persist single permission change, it needs to combine multiple things in a 
> single transaction:
> 1. Doing the actual operation (priv change)
> 2. Updating notification ID.
> 
> All the above steps are put into in a single transaction to guarantee that 
> notificationID handling is atomic.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
>  98349232bc658c39791e58b64949ecb975fff7a0 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  24729282bf17960152f87b1d3124caeafb47e6b2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  3695709e03e683afe6196def53883e37e4910a1c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java
>  2ff715f66ea6c2589a281b988438526546af3d3b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  e678b575f86cd4797ad01f12e4a60fbeec9f84f5 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  3f3afb7080ee330fedd48f4d400553fe14d57deb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
>  caa3c58b6d2e5874bea52379b9dd549a76698b9b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  f3cefd6a232bfb91db28f04bebcc98ab3c1ca658 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a35c8d7dde485cf46d61968a211d1dbb6d9d6076 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
>  1c3a4f29984379f5246da8d85fe661320c8a1043 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  d601b1e107ab3c3a4d9cc5e3038a11547182c5c9 
> 
> Diff: https://reviews.apache.org/r/55246/diff/
> 
> 
> Testing
> ---
> 
> New unit tests added in TestHMSFollower.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

[Hao Hao]

> On Jan. 6, 2017, 9:41 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  line 452
> > 
> >
> > If HMS follower just calls dropPrivilege for sentrystore, we will not 
> > be updating the  permUpdater in the SentryPlugin. Is that intentional?
> > 
> > Can you explain.

Yeah, it is intentionally, the permUpdater should be changed/or removed once 
doing SENTRY-1567. To use perisisted delta instead of in memory data.


- Hao


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


On Jan. 6, 2017, 1:37 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55246/
> ---
> 
> (Updated Jan. 6, 2017, 1:37 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1536: Refactor SentryStore transaction management to allow for extra 
> transactions for a single permission update
> 
> Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347
> 
> To persist single permission change, it needs to combine multiple things in a 
> single transaction:
> 1. Doing the actual operation (priv change)
> 2. Updating notification ID.
> 
> All the above steps are put into in a single transaction to guarantee that 
> notificationID handling is atomic.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
>  98349232bc658c39791e58b64949ecb975fff7a0 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  24729282bf17960152f87b1d3124caeafb47e6b2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  3695709e03e683afe6196def53883e37e4910a1c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java
>  2ff715f66ea6c2589a281b988438526546af3d3b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  e678b575f86cd4797ad01f12e4a60fbeec9f84f5 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  3f3afb7080ee330fedd48f4d400553fe14d57deb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
>  caa3c58b6d2e5874bea52379b9dd549a76698b9b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  f3cefd6a232bfb91db28f04bebcc98ab3c1ca658 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a35c8d7dde485cf46d61968a211d1dbb6d9d6076 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
>  1c3a4f29984379f5246da8d85fe661320c8a1043 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  d601b1e107ab3c3a4d9cc5e3038a11547182c5c9 
> 
> Diff: https://reviews.apache.org/r/55246/diff/
> 
> 
> Testing
> ---
> 
> New unit tests added in TestHMSFollower.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 54729: SENTRY-1538: Create schema for storing HMS path change and Sentry permission change.

[Hao Hao]

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

(Updated Jan. 10, 2017, 1:57 a.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
Vamsee Yarlagadda.


Repository: sentry


Description
---

Create schema for storing HMS path change and Sentry permission change. It 
contains change ID, timestamp and the change event in JSON format.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
 98349232bc658c39791e58b64949ecb975fff7a0 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java
 b66f70ba3f9cec6b44dc853ed09494be37303e2f 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
 53243b44329997e64ab70db5e5d8c3614ab974d6 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java
 PRE-CREATION 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
 ea1c8f62f300abd03206fdd9ec76fbf216fc6a2d 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java
 315d4b3a80b9ab3ac8704ecbab81876f141423f1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 fb5470f635e03c762beb5bc2c6b06641b2476ce9 

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


Testing
---


Thanks,

Hao Hao

Re: Review Request 54729: SENTRY-1538: Create schema for storing HMS path change and Sentry permission change.

[Hao Hao]

> On Jan. 6, 2017, 10:28 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java,
> >  lines 73-87
> > 
> >
> > Just curious to know how datanucleus leverages these overriden methods 
> > like hashCode, toString, equals ?

Datanucleus use hashCode, and equals to determine equality of a persistable 
object. More reference: 
http://www.datanucleus.org/products/accessplatform/jdo/orm/one_to_many_collection.html


- Hao


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


On Dec. 14, 2016, 2:35 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54729/
> ---
> 
> (Updated Dec. 14, 2016, 2:35 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Create schema for storing HMS path change and Sentry permission change. It 
> contains change ID, timestamp and the change event in JSON format.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
>  98349232bc658c39791e58b64949ecb975fff7a0 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
>  53243b44329997e64ab70db5e5d8c3614ab974d6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
>  ea1c8f62f300abd03206fdd9ec76fbf216fc6a2d 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java
>  315d4b3a80b9ab3ac8704ecbab81876f141423f1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  fb5470f635e03c762beb5bc2c6b06641b2476ce9 
> 
> Diff: https://reviews.apache.org/r/54729/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 54729: SENTRY-1538: Create schema for storing HMS path change and Sentry permission change.

[Hao Hao]

> On Jan. 6, 2017, 9:38 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java,
> >  line 82
> > 
> >
> > What is the idea here of multiplying 31 by one?
> > And why changeId is not participating in the calculation?
> > 
> > Can we just use changeId as the hashcode?
> 
> Hao Hao wrote:
> Will add changeID as the hashcode as well

Using 31 is to use an odd prime to generate the hashcode. More reference here: 
http://stackoverflow.com/questions/299304/why-does-javas-hashcode-in-string-use-31-as-a-multiplier


- Hao


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


On Dec. 14, 2016, 2:35 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54729/
> ---
> 
> (Updated Dec. 14, 2016, 2:35 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Create schema for storing HMS path change and Sentry permission change. It 
> contains change ID, timestamp and the change event in JSON format.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
>  98349232bc658c39791e58b64949ecb975fff7a0 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
>  53243b44329997e64ab70db5e5d8c3614ab974d6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
>  ea1c8f62f300abd03206fdd9ec76fbf216fc6a2d 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java
>  315d4b3a80b9ab3ac8704ecbab81876f141423f1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  fb5470f635e03c762beb5bc2c6b06641b2476ce9 
> 
> Diff: https://reviews.apache.org/r/54729/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 54729: SENTRY-1538: Create schema for storing HMS path change and Sentry permission change.

[Hao Hao]

> On Jan. 6, 2017, 9:38 p.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java,
> >  line 161
> > 
> >
> > Okease add javadocs for the new functions

Just realized this is a useless method which was added when generating json. 
Will remove it for now.


> On Jan. 6, 2017, 9:38 p.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java,
> >  line 162
> > 
> >
> > Can we define "/" as a constant (if it isn't in some hadoop libraries 
> > that we use already)?

Same as above. Will remove it.


> On Jan. 6, 2017, 9:38 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java,
> >  line 82
> > 
> >
> > What is the idea here of multiplying 31 by one?
> > And why changeId is not participating in the calculation?
> > 
> > Can we just use changeId as the hashcode?

Will add changeID as the hashcode as well


- Hao


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


On Dec. 14, 2016, 2:35 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54729/
> ---
> 
> (Updated Dec. 14, 2016, 2:35 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Create schema for storing HMS path change and Sentry permission change. It 
> contains change ID, timestamp and the change event in JSON format.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
>  98349232bc658c39791e58b64949ecb975fff7a0 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
>  53243b44329997e64ab70db5e5d8c3614ab974d6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
>  ea1c8f62f300abd03206fdd9ec76fbf216fc6a2d 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java
>  315d4b3a80b9ab3ac8704ecbab81876f141423f1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  fb5470f635e03c762beb5bc2c6b06641b2476ce9 
> 
> Diff: https://reviews.apache.org/r/54729/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Sentry-jdk-1.7 - Build # 751 - Unstable

[Apache Jenkins Server]

The Apache Jenkins build system has built Sentry-jdk-1.7 (build #751)

Status: Unstable

Check console output at https://builds.apache.org/job/Sentry-jdk-1.7/751/ to 
view the results.

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

[Hao Hao]

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

(Updated Jan. 10, 2017, 12:01 a.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(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
 bd4ada44bf882214dc20255bf902a0fbf1a0393c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 c4fdf1d25e9b669b4df1fde69961fd33f08df732 
  
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

[Hao Hao]

> On Jan. 6, 2017, 10:08 p.m., Vamsee Yarlagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  lines 160-167
> > 
> >
> > Why do we have to drop a sentry table privilege during create table?

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


> On Jan. 6, 2017, 10:08 p.m., Vamsee Yarlagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  lines 257-263
> > 
> >
> > Same question as above? Why does create commands involve in drop 
> > privileges?

Same answer as above.


> On Jan. 6, 2017, 10:08 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java,
> >  line 163
> > 
> >
> > If notificationLogEnabled is set to false, then in the new logic we 
> > simply skip setting up the hmsFollowerExecutor which is not the case in the 
> > original code. I thought we should simply fall back to the original 
> > metastore listener than the notification log listener.

Not quite follow your question here. which original code you are referring to 
here? Start of original metastore listener is not here.


- Hao


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


On Dec. 20, 2016, 12:21 a.m., Hao Hao wrote:
> 
> ---
> 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
> ---
> 
> 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
> -
> 
>   
> 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
> 
>

Sentry-jdk-1.7-v2 - Build # 52 - Still Failing

[Apache Jenkins Server]

The Apache Jenkins build system has built Sentry-jdk-1.7-v2 (build #52)

Status: Still Failing

Check console output at https://builds.apache.org/job/Sentry-jdk-1.7-v2/52/ to 
view the results.

Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true

[Hao Hao]

> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 1624
> > 
> >
> > Do you know why isMultiActionsSupported is needed? The existing comment 
> > is not quite clear to me.
> 
> Vamsee Yarlagadda wrote:
> Updated the description of the review to address your question.
> 
> Vamsee Yarlagadda wrote:
> This method is used to check which entities support multiple actions 
> (QUERY, UPDATE, ALL). As per the comment, it looks like Database and Table 
> are the ones that support all three. Do you think even Server, Column support 
> all actions? If so, we can get rid of the entire method.

I am also not quite sure here. As this code exist quite a while should we 
change its behavior before we are complete clear?


- Hao


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


On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> ---
> 
> (Updated Jan. 9, 2017, 8:15 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> 1. 
> isMultiActionsSupported used to return true for all invocations as 
> tPrivilege.getDbName() is never set to NULL. 
> So we should change it instead to do a proper check for null - 
> SentryStore.isNull() instead.
> 
> 2. Currently this method is only dealing database name string but it should 
> also check for table name strings as mentioned in the comments of the above 
> method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level 
> privileges". So fixes the method to handle tables as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  3f3afb7080ee330fedd48f4d400553fe14d57deb 
> 
> Diff: https://reviews.apache.org/r/54526/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 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true

[Vamsee Yarlagadda]

> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 1624
> > 
> >
> > Do you know why isMultiActionsSupported is needed? The existing comment 
> > is not quite clear to me.
> 
> Vamsee Yarlagadda wrote:
> Updated the description of the review to address your question.

This method is used to check which entities support multiple actions (QUERY, 
UPDATE, ALL). As per the comment, it looks like Database and Table are the ones 
that support all three. Do you think even Server, Column support all actions? 
If so, we can get rid of the entire method.


- Vamsee


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


On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> ---
> 
> (Updated Jan. 9, 2017, 8:15 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> 1. 
> isMultiActionsSupported used to return true for all invocations as 
> tPrivilege.getDbName() is never set to NULL. 
> So we should change it instead to do a proper check for null - 
> SentryStore.isNull() instead.
> 
> 2. Currently this method is only dealing database name string but it should 
> also check for table name strings as mentioned in the comments of the above 
> method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level 
> privileges". So fixes the method to handle tables as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  3f3afb7080ee330fedd48f4d400553fe14d57deb 
> 
> Diff: https://reviews.apache.org/r/54526/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 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true

[Vamsee Yarlagadda]

> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 1624
> > 
> >
> > Do you know why isMultiActionsSupported is needed? The existing comment 
> > is not quite clear to me.

Updated the description of the review to address your question.


- Vamsee


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


On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> ---
> 
> (Updated Jan. 9, 2017, 8:15 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> 1. 
> isMultiActionsSupported used to return true for all invocations as 
> tPrivilege.getDbName() is never set to NULL. 
> So we should change it instead to do a proper check for null - 
> SentryStore.isNull() instead.
> 
> 2. Currently this method is only dealing database name string but it should 
> also check for table name strings as mentioned in the comments of the above 
> method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level 
> privileges". So fixes the method to handle tables as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  3f3afb7080ee330fedd48f4d400553fe14d57deb 
> 
> Diff: https://reviews.apache.org/r/54526/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 53920: SENTRY-1534: Oracle supports serializable instead of repeatable-read

[Hao Hao]

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


Ship it!




Ship It!

- Hao Hao


On Jan. 5, 2017, 9:04 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53920/
> ---
> 
> (Updated Jan. 5, 2017, 9:04 a.m.)
> 
> 
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1534
> https://issues.apache.org/jira/browse/SENTRY-1534
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1534: Oracle supports serializable instead of repeatable-read
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  919fdaf11c36b412211176ede3c98170a2e34235 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f83d72160e1c2d3d1ec1b870ea4b4d5dda1a 
> 
> Diff: https://reviews.apache.org/r/53920/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore

[Hao Hao]

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


Ship it!




- Hao Hao


On Jan. 5, 2017, 8:28 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> ---
> 
> (Updated Jan. 5, 2017, 8:28 p.m.)
> 
> 
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1515
> https://issues.apache.org/jira/browse/SENTRY-1515
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1515: Cleanup exception handling in SentryStore
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  3695709e03e683afe6196def53883e37e4910a1c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
>  73872813fb37428529f674fef924f5a05d23c2f6 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java
>  3d756c914a1d79db15ab66eaa657c96d70e0dd1c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  2ee06f9f236694f87beb9466285bb6363a0007de 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
>  f717f38cb14ea7594f87ec6c6bf30b78241dfed6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e8d0e2505c5b178cae3d27e7d85caa652f630a2d 
> 
> Diff: https://reviews.apache.org/r/54338/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true

[Vamsee Yarlagadda]

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

(Updated Jan. 9, 2017, 8:15 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
kalvagadda.


Repository: sentry


Description (updated)
---

1. 
isMultiActionsSupported used to return true for all invocations as 
tPrivilege.getDbName() is never set to NULL. 
So we should change it instead to do a proper check for null - 
SentryStore.isNull() instead.

2. Currently this method is only dealing database name string but it should 
also check for table name strings as mentioned in the comments of the above 
method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level 
privileges". So fixes the method to handle tables as well.


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 3f3afb7080ee330fedd48f4d400553fe14d57deb 

Diff: https://reviews.apache.org/r/54526/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 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true

[Vamsee Yarlagadda]

> On Jan. 9, 2017, 6:27 p.m., Mat Crocker wrote:
> > should your || be an && ?
> > this logic suggests its ok if the db is null, but the table name isn't

The comment for this function says: "// Currently INSERT/SELECT/ALL are 
supported for Table and DB level privileges"

So if the privilege has either DB or Table set, then they support multiple 
actions. So that's why it has to be || instead of &&.


- Vamsee


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


On Jan. 5, 2017, 12:53 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> ---
> 
> (Updated Jan. 5, 2017, 12:53 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Renames the method isMultiActionsSupported to isDBNull to reflect the right 
> action being performed.
> Change based on: https://reviews.apache.org/r/54525/
> 
> Should we remove the method as the whole and do the DB null check directly at 
> the if clauses?
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  3f3afb7080ee330fedd48f4d400553fe14d57deb 
> 
> Diff: https://reviews.apache.org/r/54526/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 55094: SENTRY-1532: Sentry Web UI isn't working

[Hao Hao]

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


Fix it, then Ship it!





sentry-tests/sentry-tests-solr/pom.xml (line 58)


Can you add a comment to explain it here? Thanks!


- Hao Hao


On Jan. 5, 2017, 12:06 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55094/
> ---
> 
> (Updated Jan. 5, 2017, 12:06 a.m.)
> 
> 
> Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan 
> kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1532
> https://issues.apache.org/jira/browse/SENTRY-1532
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1532: Sentry Web UI isn't working
> 
> 
> Diffs
> -
> 
>   sentry-service/sentry-service-server/pom.xml 
> 97bd326b0305303194a965a60b6afd0feefbbe0f 
>   sentry-tests/sentry-tests-solr/pom.xml 
> a60b4eed4dde6c182b061681305013b3bd548cc3 
> 
> Diff: https://reviews.apache.org/r/55094/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 55094: SENTRY-1532: Sentry Web UI isn't working

[Alexander Kolbasov]

> On Jan. 9, 2017, 6:22 p.m., Mat Crocker wrote:
> > was the Web UI actually started and smoke tested, or just a unit test fix ?

I actually started it and accessed all entry points. Without the fix you can't 
use it.


- Alexander


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


On Jan. 5, 2017, 12:06 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55094/
> ---
> 
> (Updated Jan. 5, 2017, 12:06 a.m.)
> 
> 
> Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan 
> kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1532
> https://issues.apache.org/jira/browse/SENTRY-1532
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1532: Sentry Web UI isn't working
> 
> 
> Diffs
> -
> 
>   sentry-service/sentry-service-server/pom.xml 
> 97bd326b0305303194a965a60b6afd0feefbbe0f 
>   sentry-tests/sentry-tests-solr/pom.xml 
> a60b4eed4dde6c182b061681305013b3bd548cc3 
> 
> Diff: https://reviews.apache.org/r/55094/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore

[Mat Crocker]

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


Ship it!




looks like basic try/catch clean up, looks good to me.

- Mat Crocker


On Jan. 5, 2017, 8:28 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> ---
> 
> (Updated Jan. 5, 2017, 8:28 p.m.)
> 
> 
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1515
> https://issues.apache.org/jira/browse/SENTRY-1515
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1515: Cleanup exception handling in SentryStore
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  3695709e03e683afe6196def53883e37e4910a1c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
>  73872813fb37428529f674fef924f5a05d23c2f6 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java
>  3d756c914a1d79db15ab66eaa657c96d70e0dd1c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  2ee06f9f236694f87beb9466285bb6363a0007de 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
>  f717f38cb14ea7594f87ec6c6bf30b78241dfed6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e8d0e2505c5b178cae3d27e7d85caa652f630a2d 
> 
> Diff: https://reviews.apache.org/r/54338/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 54798: SENTRY-1546 Generic Policy provides bad error messages for Sentry exceptions

[Mat Crocker]

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



agreed with Vamsee, it needs a trimmedNamed + "_Already Exists" space before 
all messages

- Mat Crocker


On Dec. 16, 2016, 12:23 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54798/
> ---
> 
> (Updated Dec. 16, 2016, 12:23 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, 
> and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added code changes to SentryStore to have proper message (where the 
> exceptions are originated). If this is done we can avoid decorating them 
> later.
> I saw issue with exceptions created while role create/drop and made required 
> changes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  742798d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
>  898632d 
> 
> Diff: https://reviews.apache.org/r/54798/diff/
> 
> 
> Testing
> ---
> 
> I have performed tests using Sentrytool Client.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 55094: SENTRY-1532: Sentry Web UI isn't working

[Mat Crocker]

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



was the Web UI actually started and smoke tested, or just a unit test fix ?

- Mat Crocker


On Jan. 5, 2017, 12:06 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55094/
> ---
> 
> (Updated Jan. 5, 2017, 12:06 a.m.)
> 
> 
> Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan 
> kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1532
> https://issues.apache.org/jira/browse/SENTRY-1532
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1532: Sentry Web UI isn't working
> 
> 
> Diffs
> -
> 
>   sentry-service/sentry-service-server/pom.xml 
> 97bd326b0305303194a965a60b6afd0feefbbe0f 
>   sentry-tests/sentry-tests-solr/pom.xml 
> a60b4eed4dde6c182b061681305013b3bd548cc3 
> 
> Diff: https://reviews.apache.org/r/55094/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 54445: SENTRY-1547 - It is possible to create a privilege with all empty fields

[Mat Crocker]

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



minor nit, should be labeled "fields" not "feilds" 

Looks like you handled all the right cases though

- Mat Crocker


On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> ---
> 
> (Updated Dec. 14, 2016, 10:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Both the server_name and action feilds are mandatory in previlage request.
> 
> I had to take a different approach in solving the issue based on the review 
> comments. I have added validation logic in SentryPolicyStoreProcessor 
> enforcing the mandatory fields like server_name and action to be present in 
> privilege/privileges added.
> 
> If the validation is failed exception is raised and appropriate error is 
> message is sent.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java
>  15b339f 
> 
> Diff: https://reviews.apache.org/r/54445/diff/
> 
> 
> Testing
> ---
> 
> I have done unit tests using sentry cli thrift client.
> 
> 
> File Attachments
> 
> 
> SENTRY-1547.002.patch
>   
> https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>