Re: Review Request 69204: SENTRY-2437: When granting privileges a single transaction per grant causes long delays

2018-10-29 Thread Na Li via Review Board 

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


Ship it!




Ship It!

- Na Li


On Oct. 29, 2018, 9:35 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69204/
> ---
> 
> (Updated Oct. 29, 2018, 9:35 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2437
> https://issues.apache.org/jira/browse/SENTRY-2437
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry creates a transaction for each TSentryPrivilege object it 
> needs to grant. If the list of privileges is very large creating a single 
> transaction for each significantly affects performance. This is particularly 
> impactful for tables with large columns and if a user grants privileges to 
> many of those columns
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  01b363479 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  97407fff5 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  1d87b0b66 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  f2f38a38f 
> 
> 
> Diff: https://reviews.apache.org/r/69204/diff/2/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestSentryStore
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestHMSFollowerSentryStoreIntegration
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Review Request 69212: SENTRY-2329: Integrate sentry with Hadoop 3.1.1

2018-10-29 Thread Sergio Pena via Review Board 

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

Review request for sentry and kalyan kumar kalvagadda.


Bugs: sentry-2329
https://issues.apache.org/jira/browse/sentry-2329


Repository: sentry


Description
---

Bump hadoop.version to 3.1.1. The changes on Sentry are compatible with Hadoop 
2.7.


Diffs
-

  pom.xml acbdcc2722bf189811cb528ac1b2d07983a571c2 
  sentry-binding/sentry-binding-solr/pom.xml 
f2a5fca76d3d220fcf2b72a3179ff5218fc6577c 
  sentry-hdfs/sentry-hdfs-common/pom.xml 
df6f04c048b502ff5f8e8ec397d75166faba8c3c 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 c9ecc4021b167d98c7dade409c97ae7d26e967ea 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
 18b62652a11dfee6683cb8f24944ccd3d344dc9f 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithKerberos.java
 5d94d4bc6a2a47189e69556a5e4d9bdee05952a7 


Diff: https://reviews.apache.org/r/69212/diff/1/


Testing
---


Thanks,

Sergio Pena

Re: Review Request 69204: SENTRY-2437: When granting privileges a single transaction per grant causes long delays

2018-10-29 Thread Arjun Mishra via Review Board 

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

(Updated Oct. 29, 2018, 9:35 p.m.)


Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Changes
---

Added changes to revoke methods as well


Bugs: SENTRY-2437
https://issues.apache.org/jira/browse/SENTRY-2437


Repository: sentry


Description
---

Currently sentry creates a transaction for each TSentryPrivilege object it 
needs to grant. If the list of privileges is very large creating a single 
transaction for each significantly affects performance. This is particularly 
impactful for tables with large columns and if a user grants privileges to many 
of those columns


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 01b363479 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 97407fff5 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
 1d87b0b66 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 f2f38a38f 


Diff: https://reviews.apache.org/r/69204/diff/2/

Changes: https://reviews.apache.org/r/69204/diff/1-2/


Testing
---

$ mvn -f sentry-service/sentry-service-server/pom.xml test 
-Dtest=TestSentryStore
$ mvn -f sentry-service/sentry-service-server/pom.xml test 
-Dtest=TestHMSFollowerSentryStoreIntegration


Thanks,

Arjun Mishra

Review Request 69204: SENTRY-2437: When granting privileges a single transaction per grant causes long delays

2018-10-29 Thread Arjun Mishra via Review Board 

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

Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Bugs: SENTRY-2437
https://issues.apache.org/jira/browse/SENTRY-2437


Repository: sentry


Description
---

Currently sentry creates a transaction for each TSentryPrivilege object it 
needs to grant. If the list of privileges is very large creating a single 
transaction for each significantly affects performance. This is particularly 
impactful for tables with large columns and if a user grants privileges to many 
of those columns


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 01b363479 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 97407fff5 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
 1d87b0b66 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 f2f38a38f 


Diff: https://reviews.apache.org/r/69204/diff/1/


Testing
---

$ mvn -f sentry-service/sentry-service-server/pom.xml test 
-Dtest=TestSentryStore
$ mvn -f sentry-service/sentry-service-server/pom.xml test 
-Dtest=TestHMSFollowerSentryStoreIntegration


Thanks,

Arjun Mishra

Re: Review Request 69175: SENTRY-2433: Dropping object privileges does not include update of dropping user privileges

2018-10-29 Thread Na Li via Review Board 

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

(Updated Oct. 29, 2018, 5:07 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2433
https://issues.apache.org/jira/browse/sentry-2433


Repository: sentry


Description
---

1) use PermissionsUpdate.ALL_PRIVS in "1.1) NotificationProcessor.java static 
Update getPermUpdatableOnDrop(TSentryAuthorizable authorizable)" and 
"SentryPlugin.java public Update onDropSentryPrivilege(TDropPrivilegesRequest 
request)" instead of "PermissionsUpdate.ALL_ROLES"
2) check PermissionsUpdate.ALL_PRIVS instead of PermissionsUpdate.ALL_ROLES in 
UpdateableAuthzPermissions.applyPrivilegeUpdates() in 
"pUpdate.getDelPrivileges()" processing.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 c87d205 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 0f3c162 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 7b7d0e1 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 29d2256 


Diff: https://reviews.apache.org/r/69175/diff/2/

Changes: https://reviews.apache.org/r/69175/diff/1-2/


Testing
---

owner privilege tests pass and add new test for external table


Thanks,

Na Li

Review Request 69201: SENTRY-2436 Add annotations for classes that are used in binding as public

2018-10-29 Thread Xinran Tinney 

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

Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Repository: sentry


Description
---

Sentry bindings are using some of the classes in sentry.core and common. These 
classes are annotated as public


Diffs
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ActiveRoleSet.java
 c24a6cde 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
 88457c0d 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Column.java
 e36b09a1 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAction.java
 c5842d98 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java
 4ce01b2c 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Database.java
 e8dc1406 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Server.java
 41693c25 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Table.java
 5a981588 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java
 aecfe5b5 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
 761fb527 


Diff: https://reviews.apache.org/r/69201/diff/1/


Testing
---

mvn clean install


Thanks,

Xinran Tinney

Re: Review Request 69175: SENTRY-2433: Dropping object privileges does not include update of dropping user privileges

2018-10-29 Thread kalyan kumar kalvagadda via Review Board 

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


Fix it, then Ship it!




Fix it and ship it.


sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
Lines 931 (patched)


As you are verifying owner privileges in this test it is good to verify the 
presence of owner privileges after the external table is created.

I know that is indirectly tested when the user_1 is allowed to drop the 
table but it is good to have a explcit check.


- kalyan kumar kalvagadda


On Oct. 25, 2018, 9:56 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69175/
> ---
> 
> (Updated Oct. 25, 2018, 9:56 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2433
> https://issues.apache.org/jira/browse/sentry-2433
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> 1) use PermissionsUpdate.ALL_PRIVS in "1.1) NotificationProcessor.java static 
> Update getPermUpdatableOnDrop(TSentryAuthorizable authorizable)" and 
> "SentryPlugin.java public Update onDropSentryPrivilege(TDropPrivilegesRequest 
> request)" instead of "PermissionsUpdate.ALL_ROLES"
> 2) check PermissionsUpdate.ALL_PRIVS instead of PermissionsUpdate.ALL_ROLES 
> in UpdateableAuthzPermissions.applyPrivilegeUpdates() in 
> "pUpdate.getDelPrivileges()" processing.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  c87d205 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  b8f5ce7 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  7b7d0e1 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  29d2256 
> 
> 
> Diff: https://reviews.apache.org/r/69175/diff/1/
> 
> 
> Testing
> ---
> 
> owner privilege tests pass and add new test for external table
> 
> 
> Thanks,
> 
> Na Li
> 
>