Re: Review Request 69620: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

[Sergio Pena via Review Board]

> On Dec. 31, 2018, 4:48 a.m., Na Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
> > Lines 284 (patched)
> > 
> >
> > I don't think we should use 
> > "org.apache.sentry.binding.metastore.AuthorizingObjectStore" in testing 
> > because in production, the property should be set to  
> > "org.apache.hadoop.hive.metastore.ObjectStore", which is implemented by HMS.
> > 
> > SENTRY-355 "Support metadata read privilege enforcement for Metastore 
> > pluging" replaces the implementation of ObjectStore, but we are not going 
> > to use this approach.
> > 
> > The approach we decide to take is for HMS server to call Preeventlisten 
> > for authorization and filter hook to remove items that user does not have 
> > access. In HMS server, Sentry implementation of the hook (refered as 
> > , and should be SentryMetaStoreFilterHook ) is configured 
> > in following way.  
> > MetastoreConf.setClass(conf, ConfVars.FILTER_HOOK, 
> > .class,
> > MetaStoreFilterHook.class);
> > 
> > Therefore, in e2e test, we should configure HMS server to use filter 
> > hook and keep the value of HiveConf.ConfVars.METASTORE_RAW_STORE_IMPL to be 
> > default, which is "org.apache.hadoop.hive.metastore.ObjectStore". In this 
> > way, we can test the real sentry-hive integration. 
> > On the other hand, you need fix of HIVE-20776 in order to make the test 
> > work.

TestMetastoreEndToEnd.java sets the enableAuthorizingObjectStore to false to 
avoid using it on the tests. I cannot remove it unless I do more work on 
removing the AuthorizingObjectStore, but we don't know who's using it.

This patch is just another way to protect HMS.


- Sergio


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


On Dec. 21, 2018, 5:39 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69620/
> ---
> 
> (Updated Dec. 21, 2018, 5:39 p.m.)
> 
> 
> Review request for sentry and Na Li.
> 
> 
> Bugs: sentry-2483
> https://issues.apache.org/jira/browse/sentry-2483
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add READ_DATABASE and READ_TABLE events support to provide read authorization 
> to HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5c69451922e062cc3f04d37c5e7347d17f 
>   sentry-tests/sentry-tests-hive/pom.xml 
> 74777bbff590ea63c18492c77ae86042734d8e70 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e7d7d7a2e89278f1287115bf835513ef3f 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348572f0c01001b6bfa03d5ffb780f5a5e75 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304fbb9926d98939ee0aa8c74f0abc8789fa9 
> 
> 
> Diff: https://reviews.apache.org/r/69620/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 69620: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

[Sergio Pena via Review Board]

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

Review request for sentry and Na Li.


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


Repository: sentry


Description
---

Add READ_DATABASE and READ_TABLE events support to provide read authorization 
to HMS.


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5c69451922e062cc3f04d37c5e7347d17f 
  sentry-tests/sentry-tests-hive/pom.xml 
74777bbff590ea63c18492c77ae86042734d8e70 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e7d7d7a2e89278f1287115bf835513ef3f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348572f0c01001b6bfa03d5ffb780f5a5e75 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304fbb9926d98939ee0aa8c74f0abc8789fa9 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

> On Dec. 20, 2018, 6:07 p.m., Arjun Mishra wrote:
> > Sergio I don't see the binding instantiated with privilege cache. Can you 
> > please implement it? It really speeds up performance. Thoughts?
> > 
> > ===
> > HiveAuthzBinding binding = null;
> > try {
> >   binding = getHiveBindingWithPrivilegeCache(hiveAuthzBinding, 
> > context.getUserName());
> > } catch (SemanticException e) {
> >   // Will use the original hiveAuthzBinding
> >   binding = hiveAuthzBinding;
> > }
> > ===
> 
> Sergio Pena wrote:
> I'm not sure about it. The cache binding is generated on every filter 
> call, so how does the cache performns here?

Thanks for the review. Just FYI. I looked at the cache, and it has more 
overhead if we use the cache in the filter than not usingi it. The reason is 
that the filter only requires 1 hierarchy permission, and the HiveAuthzBinding 
calls Sentry one per hierarchy. So building a cache will bring more data than 
is required for only one check. We should be safe.


- Sergio


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


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2481
> https://issues.apache.org/jira/browse/sentry-2481
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
> filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
> the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
> to code duplication between SentryMetaStoreFilterHook and 
> DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created 
> that accepts different types of objects to be filtered (unit tests are added 
> to verify the cases).
> 
> 
> Diffs
> -
> 
>   .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
>   pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  c37ce646da97afb2e5c033fb3acf43190a4fae80 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
>   sentry-binding/sentry-binding-hive/pom.xml 
> b74516d70eaf873ef46914e2fbcfe08753bc1be4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  38ce2db374ee4f46190544479bc0713de2fce420 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> 

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

> On Dec. 20, 2018, 6:12 p.m., Arjun Mishra wrote:
> > Sergio, seems like we are authorizing one database or one table at a time 
> > and then adding it to the list of filtered entities. Can we authorize them 
> > collectively in a single transacation?

This would be a good idea to improve, but the current code in the 
HiveAuthzBinding.authorize() does not allow to do so. If I pass a list of 
objects to check for authorization, if at least one is denied, then the method 
will throw an AuthorizationException which it is not desired. The Hive binding 
will need to be improved to allow this.


- Sergio


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


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2481
> https://issues.apache.org/jira/browse/sentry-2481
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
> filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
> the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
> to code duplication between SentryMetaStoreFilterHook and 
> DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created 
> that accepts different types of objects to be filtered (unit tests are added 
> to verify the cases).
> 
> 
> Diffs
> -
> 
>   .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
>   pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  c37ce646da97afb2e5c033fb3acf43190a4fae80 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
>   sentry-binding/sentry-binding-hive/pom.xml 
> b74516d70eaf873ef46914e2fbcfe08753bc1be4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  38ce2db374ee4f46190544479bc0713de2fce420 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

> On Dec. 20, 2018, 6:07 p.m., Arjun Mishra wrote:
> > Sergio I don't see the binding instantiated with privilege cache. Can you 
> > please implement it? It really speeds up performance. Thoughts?
> > 
> > ===
> > HiveAuthzBinding binding = null;
> > try {
> >   binding = getHiveBindingWithPrivilegeCache(hiveAuthzBinding, 
> > context.getUserName());
> > } catch (SemanticException e) {
> >   // Will use the original hiveAuthzBinding
> >   binding = hiveAuthzBinding;
> > }
> > ===

I'm not sure about it. The cache binding is generated on every filter call, so 
how does the cache performns here?


- Sergio


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


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2481
> https://issues.apache.org/jira/browse/sentry-2481
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
> filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
> the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
> to code duplication between SentryMetaStoreFilterHook and 
> DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created 
> that accepts different types of objects to be filtered (unit tests are added 
> to verify the cases).
> 
> 
> Diffs
> -
> 
>   .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
>   pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  c37ce646da97afb2e5c033fb3acf43190a4fae80 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
>   sentry-binding/sentry-binding-hive/pom.xml 
> b74516d70eaf873ef46914e2fbcfe08753bc1be4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  38ce2db374ee4f46190544479bc0713de2fce420 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

> On Dec. 20, 2018, 6:06 p.m., Arjun Mishra wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
> > Lines 184 (patched)
> > 
> >
> > We should throw an AuthorizationException with message here. Otherwise 
> > the expected privileges won't be printed out on Hive console

Not possible. This filter is for the HMS server, and the denied authorization 
is to let the user know that the object does not exist. The reason is to avoid 
users to guess which objects exist and which do not.


- Sergio


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


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2481
> https://issues.apache.org/jira/browse/sentry-2481
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
> filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
> the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
> to code duplication between SentryMetaStoreFilterHook and 
> DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created 
> that accepts different types of objects to be filtered (unit tests are added 
> to verify the cases).
> 
> 
> Diffs
> -
> 
>   .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
>   pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  c37ce646da97afb2e5c033fb3acf43190a4fae80 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
>   sentry-binding/sentry-binding-hive/pom.xml 
> b74516d70eaf873ef46914e2fbcfe08753bc1be4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  38ce2db374ee4f46190544479bc0713de2fce420 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

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

(Updated Dec. 20, 2018, 3:45 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Changes
---

Call the new MetastoreAuthzObjectFitler from AuthorizingObjectStore classes.


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


Repository: sentry


Description
---

Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
to code duplication between SentryMetaStoreFilterHook and 
DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created that 
accepts different types of objects to be filtered (unit tests are added to 
verify the cases).


Diffs (updated)
-

  .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
  pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
 c37ce646da97afb2e5c033fb3acf43190a4fae80 
  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
  sentry-binding/sentry-binding-hive/pom.xml 
b74516d70eaf873ef46914e2fbcfe08753bc1be4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 38ce2db374ee4f46190544479bc0713de2fce420 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java
 92eb1366be44bd53f57e0900634b1cb4eae6470e 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
 d015085c71822c34a3315dc884596acc8ee2421a 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHiveMetaStoreClient.java
 e30a86050a23a69cb9d613ec3500a1915974ed65 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
 5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 PRE-CREATION 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
 bcd1fa2351f7e7928f5499aa5f86906640f62504 


Diff: https://reviews.apache.org/r/69586/diff/3/

Changes: https://reviews.apache.org/r/69586/diff/2-3/


Testing
---

Added unit tests for the SentryMetaStoreFilterHook.


Thanks,

Sergio Pena

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

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

(Updated Dec. 19, 2018, 3:24 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Changes
---

Addressed lina's feedback.


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


Repository: sentry


Description
---

Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
to code duplication between SentryMetaStoreFilterHook and 
DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created that 
accepts different types of objects to be filtered (unit tests are added to 
verify the cases).


Diffs (updated)
-

  .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
  pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
 c37ce646da97afb2e5c033fb3acf43190a4fae80 
  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
  sentry-binding/sentry-binding-hive/pom.xml 
b74516d70eaf873ef46914e2fbcfe08753bc1be4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 38ce2db374ee4f46190544479bc0713de2fce420 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
 5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 PRE-CREATION 


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

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


Testing
---

Added unit tests for the SentryMetaStoreFilterHook.


Thanks,

Sergio Pena

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

> On Dec. 18, 2018, 7:52 p.m., Na Li wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
> > Lines 67 (patched)
> > 
> >
> > If the object is not a table, then this would be wrong. Is there a type 
> > field to check it is indeed a table?

Good catch. I don't think is necessary to check if it is a table or not because 
the code will never call the filterTable with an object that is not a table, 
but it would be good to check just in case. I found that getType() returns the 
type of the object.


> On Dec. 18, 2018, 7:52 p.m., Na Li wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
> > Lines 115 (patched)
> > 
> >
> > should we keep the username case? see 
> > https://issues.apache.org/jira/browse/SENTRY-2432

True.


- Sergio


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


On Dec. 18, 2018, 7:28 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 18, 2018, 7:28 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2481
> https://issues.apache.org/jira/browse/sentry-2481
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
> filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
> the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
> to code duplication between SentryMetaStoreFilterHook and 
> DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created 
> that accepts different types of objects to be filtered (unit tests are added 
> to verify the cases).
> 
> 
> Diffs
> -
> 
>   .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
>   pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  c37ce646da97afb2e5c033fb3acf43190a4fae80 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
>   sentry-binding/sentry-binding-hive/pom.xml 
> b74516d70eaf873ef46914e2fbcfe08753bc1be4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  38ce2db374ee4f46190544479bc0713de2fce420 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/1/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Sergio Pena via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
to code duplication between SentryMetaStoreFilterHook and 
DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created that 
accepts different types of objects to be filtered (unit tests are added to 
verify the cases).


Diffs
-

  .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
  pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
 c37ce646da97afb2e5c033fb3acf43190a4fae80 
  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
  sentry-binding/sentry-binding-hive/pom.xml 
b74516d70eaf873ef46914e2fbcfe08753bc1be4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 38ce2db374ee4f46190544479bc0713de2fce420 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
 5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 PRE-CREATION 


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


Testing
---

Added unit tests for the SentryMetaStoreFilterHook.


Thanks,

Sergio Pena

Re: Review Request 69530: SENTRY-2476: Optimize deleting specific paths for objects

[Sergio Pena via Review Board]

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



Besides the questions below, I have extra feedback about:
- add comments about how and why you're doing things (including the method 
header parameters)
- take care of the coding convention. use spaces where they need to be, like 
CollectionauthzPathsMapping


sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3451-3455 (patched)


Why to use two different methods instead of using only the compact? Doesn't 
do the same thing?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3485 (patched)


I did not understand this. Why do you to read all the MAuthzPathsMapping 
objects? That might be a huge number considering the thousand of objects (or 
millions) that HMS can have and that several snapshots may be stored in the DB 
due to old bugs. This would read all of them.

Isn't it going to perform bad?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3590 (patched)


This method reads all MAuthzPathsMapping object to check only if there are 
multiple MAuthzPathsMapping relations to a set of MAuthzPath objects. 

Is there a way to check that with less data read from the db? Take into 
account that there might be thousdans of MAuthzPathsMapping.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3592-3595 (patched)


This code is repeated in the Exhaustive methods. Isn't it better to pass 
the snapshotID as parameter instead? You can get the snapshotID once from the 
...Core() method.


- Sergio Pena


On Dec. 11, 2018, 8:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 11, 2018, 8:16 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2476
> https://issues.apache.org/jira/browse/SENTRY-2476
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we process a drop partition event, we fetch each path object 
> for paths_mapping object then find the one we want to delete and then delete 
> it. We should avoid fetching all objects and directly delete the path that 
> needs to be deleted
> 
> This affects, deleteAuthzPathsMapping, renameAuthzPathsMapping, 
> updateAuthzPathsMapping, and addAuthzPathsMappingCore methods that are known 
> at the moment
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d701 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610 
> 
> 
> Diff: https://reviews.apache.org/r/69530/diff/5/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestNotificationProcessor
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Nov. 29, 2018, 10:14 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 29, 2018, 10:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d70145474c173fd4abfd2d2189e729e170c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/11/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69352: SENTRY-2452: Change the thrift interface to send the list of authorizable to sentry server

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Dec. 7, 2018, 3:14 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69352/
> ---
> 
> (Updated Dec. 7, 2018, 3:14 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2452
> https://issues.apache.org/jira/browse/SENTRY-2452
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are 
> used to send export and import requests to sentry server should be changed to 
> be able to send a list authorizables for which permission information has to 
> be exported/imported.
> 
> These requests should accommodate source and target cluster information as 
> well.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
>  6eb85217b9229438b9adbd1546a408dc507f1645 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java
>  13e57e04dd779825e2986b1e527f4e556271a162 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java
>  21f3bdf5e304fe2f49684c999d0aa0e0362320de 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java
>  cea868f0524c25bf63cfb7c532829354a280df28 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  83393a98df5e10b324f74dc12f10c20bc5f02651 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  68d864cfbdf18057d87a65a04af8991292aadccf 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  3364648dc88acbb8de1cc925fe8c512f34a4b064 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  2dc0975f482f760c18c438c1418630a7ef6a4cbb 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  b9e3bf2921a0696da639e1b1bee5d83cf2b9cee0 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
>  cf1fdab382034c8950da2613ef9b0cf1af912e33 
> 
> 
> Diff: https://reviews.apache.org/r/69352/diff/5/
> 
> 
> Testing
> ---
> 
> Made sure that existiung tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69352: SENTRY-2452: Change the thrift interface to send the list of authorizable to sentry server

[Sergio Pena via Review Board]

> On Dec. 6, 2018, 4:13 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
> > Line 298 (original), 298 (patched)
> > 
> >
> > Are you making this incompatible with Sentry 2.1?
> 
> kalyan kumar kalvagadda wrote:
> yes.

Any reasons why? The export command has been since Sentry 1.6. Shouldn't we 
keep this compatible with 2.1 and 2.0 as well?


- Sergio


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


On Dec. 6, 2018, 8:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69352/
> ---
> 
> (Updated Dec. 6, 2018, 8:23 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2452
> https://issues.apache.org/jira/browse/SENTRY-2452
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are 
> used to send export and import requests to sentry server should be changed to 
> be able to send a list authorizables for which permission information has to 
> be exported/imported.
> 
> These requests should accommodate source and target cluster information as 
> well.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
>  6eb85217b9229438b9adbd1546a408dc507f1645 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java
>  13e57e04dd779825e2986b1e527f4e556271a162 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java
>  21f3bdf5e304fe2f49684c999d0aa0e0362320de 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java
>  cea868f0524c25bf63cfb7c532829354a280df28 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  83393a98df5e10b324f74dc12f10c20bc5f02651 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  68d864cfbdf18057d87a65a04af8991292aadccf 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  3364648dc88acbb8de1cc925fe8c512f34a4b064 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  2dc0975f482f760c18c438c1418630a7ef6a4cbb 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  b9e3bf2921a0696da639e1b1bee5d83cf2b9cee0 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
>  cf1fdab382034c8950da2613ef9b0cf1af912e33 
> 
> 
> Diff: https://reviews.apache.org/r/69352/diff/4/
> 
> 
> Testing
> ---
> 
> Made sure that existiung tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69484: SENTRY-2460: Export sentry permission information to HDFS location

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Dec. 6, 2018, 9:19 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69484/
> ---
> 
> (Updated Dec. 6, 2018, 9:19 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2460
> https://issues.apache.org/jira/browse/SENTRY-2460
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry should be able to export permission information to a HDFS location.
> 
> This can be done using hadoop-common library.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
>  b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryPolicyFileFormatter.java
>  4f465b3671c1fdd6de7cd0773a29108af40311c8 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java
>  5f1e3e916d71c51e19d3d37ba788f902ed6b7e27 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
>  7baeee17339d29576b841480e600a3cfaf14adf3 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
>  cf1fdab382034c8950da2613ef9b0cf1af912e33 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
>  df9299c5df26c891ceebbc59b79dd0f7cd3ceeb4 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  b048989b42bde0318c6d0fa0e9353a2a59954407 
> 
> 
> Diff: https://reviews.apache.org/r/69484/diff/3/
> 
> 
> Testing
> ---
> 
> Added new tests to verify the new behavior added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69353: SENTRY-2454: Add new sentrys tore api to gather the privileges for a list of authorizables.

[Sergio Pena via Review Board]

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



I'd like to see the part of the SentryPolicyStoreProcessor with this patch to 
give me a bette idea of how both classes will work together. I saw that the 
previous export method requires role -> privileges mapping, but this returns 
only the list of privileges. I don't know what's the idea, but if you write the 
code for SentryPolicyStoreProcessor too would be helpful.

- Sergio Pena


On Nov. 15, 2018, 9:06 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Nov. 15, 2018, 9:06 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2454
> https://issues.apache.org/jira/browse/SENTRY-2454
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New sentry API should be implemented to fetch the privileges granted to 
> authorizables and it's children. authorizables include database, tables, 
> columns and URI's.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  e48eea377b842475f72b6fab4567a82c8fd93098 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69353/diff/1/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69352: SENTRY-2452: Change the thrift interface to send the list of authorizable to sentry server

[Sergio Pena via Review Board]

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




sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Lines 107-108 (patched)


What if objectTrimmed is empty? Should we add a check and continue with the 
loop if so? This could happen if the 'objects' object is db=db1,



sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Lines 108 (patched)


Shouldn't we support the server as well? Sentry has support for it, so 
perhaps we should do the same thing for the export commands?



sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Line 106 (original), 110 (patched)


This may throw an exception if the value is empty. The message would be 
only 'Value cannot be empty'. Should we catch that and throw a better message 
specified which key does not have a value?

An empty key may throw it too, but if you add the empty check after you 
trim the object and continue the loop, then keys won't never be empty at this 
point.



sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Lines 120-123 (patched)


is the expectation of the method to just log errors instead of throwing an 
exception? I feel we should make the contract of this method to throw an 
exception if at least one of the KeyValue are invalid.

That helps you do better testing here, and avoid an admin had to go to the 
logs to find why a certain value was not exported.



sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Line 298 (original), 298 (patched)


Are you making this incompatible with Sentry 2.1?



sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 299-300 (patched)


I don't see a usage of these two versions yet. Should we remove them and 
discuss what is the real intention?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Line 1333 (original), 1332 (patched)


request.getAuthorizables() may return a null value as this is optional in 
thrift.


- Sergio Pena


On Dec. 5, 2018, 11:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69352/
> ---
> 
> (Updated Dec. 5, 2018, 11:23 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2452
> https://issues.apache.org/jira/browse/SENTRY-2452
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are 
> used to send export and import requests to sentry server should be changed to 
> be able to send a list authorizables for which permission information has to 
> be exported/imported.
> 
> These requests should accommodate source and target cluster information as 
> well.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
>  6eb85217b9229438b9adbd1546a408dc507f1645 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java
>  13e57e04dd779825e2986b1e527f4e556271a162 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java
>  21f3bdf5e304fe2f49684c999d0aa0e0362320de 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java
>  cea868f0524c25bf63cfb7c532829354a280df28 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  83393a98df5e10b324f74dc12f10c20bc5f02651 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  68d864cfbdf18057d87a65a04af8991292aadccf 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  

Re: Review Request 69484: SENTRY-2460: Export sentry permission information to HDFS location

[Sergio Pena via Review Board]

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




sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Line 62 (original), 66 (patched)


I think this line goes in the resourcePath line, right?



sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Lines 94-96 (patched)


Does it accept relative paths (especially in the local filesystem)?



sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Line 101 (original), 113 (patched)


I know this line was already here, but don't you think that printing the 
contents of the file in the log is unsecure and for big contents the log will 
be too verbose?

Just after the line is logged, the same contents is written to the file, so 
there is duplicated values in two files. Should we remove the contents from be 
sent to the logs?



sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Lines 115-118 (patched)


Is the fileSystem closed if an exception is thrown on the br.write() and 
br.close()? Should these line be called inside a try-with-resources block?



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 72 (patched)


Is this assert needed? I think the createTempDir() throws an exception if 
the directory is not created, so this is not necessary.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 74 (patched)


Same question, is this assert needed? Don't you just need to call mkddirs() 
only?



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 77 (patched)


Why do we need 2 nodes for testing an export operation? MiniDFS clusters 
add time on running tests, the less we use them or less resources we ask, the 
test will be faster.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 87 (patched)


When is baseDir a null in this environment? the setupClass should not 
create a null baseDir, shouldn't it?



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 87-92 (patched)


When is baseDir a null in this environment? the setupClass should not 
create a null baseDir, shouldn't it?

Same question for dfsCluster.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Line 174 (original), 224 (patched)


Change the test case name to testImportExportFromLocalFile() so we 
understand the difference between this and testImportExportFromHDFS()



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 272 (patched)


Isn't it better to run resourcePath.toUri().toString() once and keep the 
value in a string before starting the tests? It is more readable + the string 
is not being generated every time it is called.


- Sergio Pena


On Dec. 5, 2018, 10:18 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69484/
> ---
> 
> (Updated Dec. 5, 2018, 10:18 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2460
> https://issues.apache.org/jira/browse/SENTRY-2460
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry should be able to export permission information to a HDFS location.
> 
> This can be done using hadoop-common library.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
>  b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
>   
> 

Re: Review Request 69285: Signal Handle Unregister

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Nov. 19, 2018, 4:32 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69285/
> ---
> 
> (Updated Nov. 19, 2018, 4:32 p.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2444
> https://issues.apache.org/jira/browse/SENTRY-2444
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2444: Signal Handle Unregister
> 
> Single unregister of function from the signal handler.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SigUtils.java
>  d621c74c2 
> 
> 
> Diff: https://reviews.apache.org/r/69285/diff/2/
> 
> 
> Testing
> ---
> 
> Build and Unit tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 69415: SENTRY-2463: Revoking ALL or * should revoke any other privilege on the entity

[Sergio Pena via Review Board]

> On Nov. 30, 2018, 5:46 p.m., kalyan kumar kalvagadda wrote:
> > I understand there is a gap here but we should be carefull in changing the 
> > current behavior.I assume, the idea here is to find a way to remove all the 
> > privileges granted to an authrozable(server/database/table)
> > 
> > Using "ALL" to soleve is not backward compatable. There could be customers 
> > who want to remove only "ALL" privilege and nothing else when "ALL" is 
> > revoked. Instead we should try to solve this with out using "ALL".
> 
> Arjun Mishra wrote:
> Thanks Kalyan. I'll put this in the backburner.

How do other databases work with the ALL privilege?


- Sergio


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


On Nov. 30, 2018, 5:21 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69415/
> ---
> 
> (Updated Nov. 30, 2018, 5:21 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2463
> https://issues.apache.org/jira/browse/SENTRY-2463
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now if we initially grant ALL and revoke SELECT or INSERT, the 
> privilege gets "modified" to INSERT or SELECT. However, conversely if we 
> initially grant SELECT or INSERT and revoke ALL, no privileges are dropped
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f392 
> 
> 
> Diff: https://reviews.apache.org/r/69415/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestDatabaseProvider
> mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Sergio Pena via Review Board]

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


Fix it, then Ship it!




It looks good, just add to the warning message that the cache will be reloaded 
later in the next time interval or something. The reason is that if a user 
finds a warning messge, they expect an answer of what to do, right?


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
Lines 140 (patched)


If this is a warning, should the user expect to do something if tihs 
message appears on the log? If not, perhaps you should add to the message that 
the cache will be reloaded in the next refresh interval (in X seconds?).


- Sergio Pena


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Review Request 69349: SENTRY-2457: Reuse connection objects on TestConcurrentClients#testConcurrentHS2Client

[Sergio Pena via Review Board]

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

Review request for sentry and Arjun Mishra.


Bugs: semtry-2457
https://issues.apache.org/jira/browse/semtry-2457


Repository: sentry


Description
---

Re-use the Connection object on the 
TestConcurrentClient#testConcurrentHS2Client method. This increase the speed of 
the test 10s.


Diffs
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java
 2da6c6b8231ff2e59c302c1ecc857f852d7a28ee 


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


Testing
---


Thanks,

Sergio Pena

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

[Sergio Pena via Review Board]

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

(Updated Nov. 5, 2018, 8 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description (updated)
---

Bump hadoop.version to 3.1.1.

This integration is compatible with old Hadoop 2.x versions. You can compile 
with Hadoop 2.x by
changing the pom.xml or you can add the Sentry/HDFS binding jars built with 
Hadoop3 in the 
Hadoop 2 classpath. Both ways are verified and working.


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/2/


Testing
---


Thanks,

Sergio Pena

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

[Sergio Pena via Review Board]

> On Nov. 1, 2018, 9:26 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
> > Line 143 (original)
> > 
> >
> > why do you remove it? Is it impossible to throw this exception?

It's not needed. The change is not using URIUtil anymore, so there is no URI 
exception.


- Sergio


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


On Oct. 30, 2018, 2:16 a.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69212/
> ---
> 
> (Updated Oct. 30, 2018, 2:16 a.m.)
> 
> 
> 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 69212: SENTRY-2329: Integrate sentry with Hadoop 3.1.1

[Sergio Pena via Review Board]

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

(Updated Nov. 2, 2018, 6:40 p.m.)


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 (updated)
-

  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/2/

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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Sergio Pena via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 37 (patched)


This is already defined in the package.jdo. Does it need to be defined here 
as well? My understanding is that it can be either on the package.jdo or the 
class file or both, but should we be consistent where we put these declarations?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 36 (original), 42 (patched)


Is this variable needed now that you remove the getters and setters?

Btw, why isn't this variable used anymore?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 259-260 (original), 259-260 (patched)


There are weird characters at the end of these lines.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 259-261 (original), 259-261 (patched)


What is the strategy for the ID? Will it continue using the increment 
strategy?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 300 (patched)


Why a new class that points to the same table as MPath is needed?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 301 (patched)


with our?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 303-311 (patched)


There are weird characters at the end of these lines.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 303-311 (patched)


There are weird characters at the end of these lines.


- Sergio Pena


On Oct. 31, 2018, 11:28 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Oct. 31, 2018, 11:28 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPathToPersist.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  20ec0deab6b97065cfe99beea3d14a6c7268aac3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  33c40613a05f7c7fde314af6aba6b269bf6ffaae 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  66db6ae9a436b9728fb3c2ebdd21167ef042f937 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/3/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Oct. 29, 2018, 4:45 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69201/
> ---
> 
> (Updated Oct. 29, 2018, 4:45 p.m.)
> 
> 
> 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
> 
>

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

[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 68926: SENTRY-2372: SentryStore should not implement grantOptionCheck

[Sergio Pena via Review Board]

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

(Updated Oct. 22, 2018, 7:16 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

Make the grant option check on the SentryPolicyStoreProcessor so that it gets 
the group information from it. The SentryStore has the new method to check the 
grant option in the DB only.


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
 b225af0bc4e8c472e2ee17677a74525b882e214c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 b8f5ce7dbc8cd2dd6317f02cc09a225ef460acf4 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c388689b63d5db7d71cd6cc47952d647bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreUtils.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
 eba40d2ebd9778ddf74df0d35ad8b22500717f73 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22e40b8958395e1c12af12272b89a778726 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 6217719b8cf0c120b47a1612f6b3f0bbba98f724 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessorTestUtils.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 16ff79e2e4258c20a46ffcf162872b5b20d97aba 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
 a8868364437ac637ba4a9ad551408de0a9304984 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 4a9afe303672baff39be01d4f190034b2bfb75fe 


Diff: https://reviews.apache.org/r/68926/diff/5/

Changes: https://reviews.apache.org/r/68926/diff/4-5/


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68926: SENTRY-2372: SentryStore should not implement grantOptionCheck

[Sergio Pena via Review Board]

> On Oct. 22, 2018, 3:40 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 356 (patched)
> > 
> >
> > is it possible p2.action is null? If so, need to check that first 
> > before comparing.

It is not possible. TSentryPrivilege marks as a required value and it 
initializes the value to an empty string.


- Sergio


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


On Oct. 20, 2018, 7:06 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68926/
> ---
> 
> (Updated Oct. 20, 2018, 7:06 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-2372
> https://issues.apache.org/jira/browse/SENTRY-2372
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Make the grant option check on the SentryPolicyStoreProcessor so that it gets 
> the group information from it. The SentryStore has the new method to check 
> the grant option in the DB only.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
>  b225af0bc4e8c472e2ee17677a74525b882e214c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  b8f5ce7dbc8cd2dd6317f02cc09a225ef460acf4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c388689b63d5db7d71cd6cc47952d647bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
>  eba40d2ebd9778ddf74df0d35ad8b22500717f73 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22e40b8958395e1c12af12272b89a778726 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  6217719b8cf0c120b47a1612f6b3f0bbba98f724 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessorTestUtils.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  16ff79e2e4258c20a46ffcf162872b5b20d97aba 
> 
> 
> Diff: https://reviews.apache.org/r/68926/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68973: SENTRY-2305: Optimize time taken for persistence HMS snapshot by persisting in parallel

[Sergio Pena via Review Board]

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




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
Lines 152 (patched)


how can we make this variable name to not be confused with threads fetching 
an hms snapshot?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
Lines 153 (patched)


is there an optimal value higher than 2 to make it faster?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3476-3479 (original), 3491-3494 (patched)


I wonder how much memory do we expect in the worst case scenarios to be 
hold in memory for all these objects in the pool?

In the previous patchthe objects were persisted (1 at a time), and GC could 
collected them.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3478 (original), 3493 (patched)


This is executed in a different transaction?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3481 (original), 3496 (patched)


Shouldn't we destroy or stop the persisting process of the rest of the 
paths if the persistingSnapshotFailed is true?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3510 (patched)


Aren't these deleted during the rollback?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 5106-5112 (patched)


How often does this code get ID conflicts while persisting in a threaded 
process? Isn't this going to make the persisting slow?


- Sergio Pena


On Oct. 22, 2018, 2:42 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68973/
> ---
> 
> (Updated Oct. 22, 2018, 2:42 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2305
> https://issues.apache.org/jira/browse/SENTRY-2305
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have considered multiple options. Persisting in batches is not an option 
> with out changing the schema as the data nucleus does not persist row in 
> batches for tables which have foreign key on other tables.
> 
> I see that best option is to persist the paths in parallel. It gave good 
> results.
> 
> Solution Approach:
> 
> I have used a thread pool to persist the snapshot. Size of this thread pool 
> is configurable. Paths for each object database/table are submitted to this 
> thread pool. If for reason some of the paths are not pesisted, snapshot is 
> removed and exception is throw back.
> 
>   This patch along with SENTRY-2423 was 5 times faster when tested with below.
> 
>  
> 
> Object Type   Count
> Databases 209
> Tables2100
> Partitions24
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22e40b8958395e1c12af12272b89a778726 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe303672baff39be01d4f190034b2bfb75fe 
> 
> 
> Diff: https://reviews.apache.org/r/68973/diff/5/
> 
> 
> Testing
> ---
> 
> Added new test and also made sure that existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68926: SENTRY-2372: SentryStore should not implement grantOptionCheck

[Sergio Pena via Review Board]

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

(Updated Oct. 20, 2018, 7:06 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

Make the grant option check on the SentryPolicyStoreProcessor so that it gets 
the group information from it. The SentryStore has the new method to check the 
grant option in the DB only.


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
 b225af0bc4e8c472e2ee17677a74525b882e214c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 b8f5ce7dbc8cd2dd6317f02cc09a225ef460acf4 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c388689b63d5db7d71cd6cc47952d647bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
 eba40d2ebd9778ddf74df0d35ad8b22500717f73 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22e40b8958395e1c12af12272b89a778726 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 6217719b8cf0c120b47a1612f6b3f0bbba98f724 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessorTestUtils.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 16ff79e2e4258c20a46ffcf162872b5b20d97aba 


Diff: https://reviews.apache.org/r/68926/diff/4/

Changes: https://reviews.apache.org/r/68926/diff/3-4/


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68926: SENTRY-2372: SentryStore should not implement grantOptionCheck

[Sergio Pena via Review Board]

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

(Updated Oct. 20, 2018, 1:58 a.m.)


Review request for sentry and kalyan kumar kalvagadda.


Changes
---

Addressed comments and add unit tests.


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


Repository: sentry


Description
---

Make the grant option check on the SentryPolicyStoreProcessor so that it gets 
the group information from it. The SentryStore has the new method to check the 
grant option in the DB only.


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
 b225af0bc4e8c472e2ee17677a74525b882e214c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 b8f5ce7dbc8cd2dd6317f02cc09a225ef460acf4 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c388689b63d5db7d71cd6cc47952d647bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
 eba40d2ebd9778ddf74df0d35ad8b22500717f73 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22e40b8958395e1c12af12272b89a778726 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 6217719b8cf0c120b47a1612f6b3f0bbba98f724 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessorTestUtils.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 16ff79e2e4258c20a46ffcf162872b5b20d97aba 


Diff: https://reviews.apache.org/r/68926/diff/3/

Changes: https://reviews.apache.org/r/68926/diff/2-3/


Testing
---


Thanks,

Sergio Pena

Re: Review Request 69076: SENTRY-2301: Log where sentry stands in the snapshot fetching process, periodically

[Sergio Pena via Review Board]

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



I did a quick look and I feel this logging will be too verbose and may slow the 
fetch process as there is one message per task executed. There can be thousands 
or even millions of tasks (one per databse, one per table, one per 100 
partitions).

Is there another way to print a progress instead of two messages per task?

- Sergio Pena


On Oct. 18, 2018, 9:29 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69076/
> ---
> 
> (Updated Oct. 18, 2018, 9:29 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When sentry is fetching snapshot from HMS, it should log periodically on 
> where it stands in the snapshot process. This will help person debugging it 
> and help him understand the progress.
> 
>  
> 
> This is important as this process could take magnitude of minutes when the 
> HMS data is huge.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  214d78c53 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  3e27d1bbe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4baeb6725 
> 
> 
> Diff: https://reviews.apache.org/r/69076/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69030: SENTRY-2427: Use Hadoop KerberosName class to derive shortName

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Oct. 16, 2018, 4:17 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69030/
> ---
> 
> (Updated Oct. 16, 2018, 4:17 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2427
> https://issues.apache.org/jira/browse/SENTRY-2427
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry doesn't use auth to local group mapping hadoop configuration. We may 
> have a use case for cross realm users to have access to sentry service and in 
> which case Sentry needs to have access to those configurations. Switching to 
> using KerberosName will handle that case and other cases as well
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
>  d2d85d3a2 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69030/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Sergio Pena via Review Board]

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


Ship it!




Looks good. Just one quick fix, can you put the progress message in a method? 
The code has two parts with the same duplicatd message and math calculations.

- Sergio Pena


On Oct. 16, 2018, 9:55 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 16, 2018, 9:55 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Sergio Pena via Review Board]

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



Could you test this log message and paste the real messages here?


sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3436-3439 (patched)


Initialize these variables before the setDetachAllOnCommit() and make more 
space lines to make the code more readable.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3442 (patched)


isn't lastProgressTime a better name than initialTime? I initially thought 
it was the first time it was taken, but then I see this is updated on every 
print.

I don't like to make comments on variables names, but this name does not 
correspond to the actual use of the variable.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3444-3449 (patched)


Move the logging block after the objects has been persisted and the 
counters updated so that it displays the actual completion of the process.

It will be likely that the log message will never be 100% because it is 
printed before the actual persisting process.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3460-3462 (patched)


Could you display the progress in %? Use the objectsPersistedCount for that.

Btw, sorry I told you about using HMS snapshot message, but this method 
references to that as 'Full paths', so it would be good to make that change to 
be consistent.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3463 (patched)


Agree, the timestamp is already part of the log message, so this is just 
duplicating a value.


- Sergio Pena


On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 12, 2018, 2 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68918: SENTRY-2423: Increase the allocation size for auto-increment of id's for Snapshot tables.

[Sergio Pena via Review Board]

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


Ship it!




Looks good

- Sergio Pena


On Oct. 3, 2018, 8:24 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68918/
> ---
> 
> (Updated Oct. 3, 2018, 8:24 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2423
> https://issues.apache.org/jira/browse/SENTRY-2423
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry uses native strategy for auto generation of ID columns for 
> which the allocation can not be increased.
> This should be change to "increment" strategy and which lets to configure the 
> allocation size. This reduces the delay caused for checking the 
> SEQUENCE_TABLE.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df46266d72f1eb3706b948fb163a44c4a1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  6539e33e021e0f5acaa7827356a8e9b3e4286b18 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e381bc7be81a4673d12c1ac9f81195c47 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a299e008cc0df92f3d692024c7794aa494b64d2d 
> 
> 
> Diff: https://reviews.apache.org/r/68918/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68547: SENTRY-2370: Create a cache of PathUpdates to send to NN

[Sergio Pena via Review Board]

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



Did you look at the Guava cache?

Read this part of using soft values in the Guava cache (btw, it has invalidate 
vs cleanUp methods which means different things):
http://blog.jessitron.com/2011/10/keeping-your-cache-down-to-size.html


I was looking at the Guava cache and it will be valuable resource for using it 
in this cache instead of doing our own thing. It is much simpler and less prone 
to errors. Really useful.


sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 139 (patched)


If imageCache.get() is not null, then it might be null after the if 
condition. The reason is GC could remove it from memory if it is not a 
hard-reference.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 140-145 (patched)


Invalidate or cleanUp? I like to separate the verbs. Invalidate means the 
current cache is not valid and it will be replaced (but not necessary removed 
from memory). Clean is what is says, clean from memory.

Also, I think we should see the future of HDFS Federation. We can get a 
benefit if we avoid cleaning the cache when multiple NN request images from 
Sentry. This way Sentry will return the cache image.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 159 (original), 216-220 (patched)


In the if() above, if imageNum != than the cache, then that will return 
null. When is this if going to be valid? imageNum will be teh same than cache.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 222 (patched)


Just check if they're different. It is a simple case.  A cache is not more 
valid if the seq num is different (higher or lower, it does not matter). This 
logic is assumming HDFS will never request a lower value. We should not assume 
that.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 248-266 (patched)


Is there a way to mock the cache objects instead of having new methods to 
get its internal value? Or mock the imageRetriever and verify it was called 
when a cache is not valid.


- Sergio Pena


On Oct. 5, 2018, 7:11 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68547/
> ---
> 
> (Updated Oct. 5, 2018, 7:11 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2370
> https://issues.apache.org/jira/browse/SENTRY-2370
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When NN requests path updates from sentry and if it exceeds the time 
> threshold, on consecutive attempts sentry will attempt to fetch the full 
> update from scratch. Instead it should cache it and update the cache before 
> sending it to NN
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  f86ce6f83 
> 
> 
> Diff: https://reviews.apache.org/r/68547/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68926: SENTRY-2372: SentryStore should not implement grantOptionCheck

[Sergio Pena via Review Board]

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

(Updated Oct. 5, 2018, 2:23 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

Make the grant option check on the SentryPolicyStoreProcessor so that it gets 
the group information from it. The SentryStore has the new method to check the 
grant option in the DB only.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c388689b63d5db7d71cd6cc47952d647bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1722109e381bc7be81a4673d12c1ac9f81195c47 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 3a68eb6bfa0224d1bce17f0bfccb008ab9b291f2 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreUtils.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
 a8868364437ac637ba4a9ad551408de0a9304984 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 a299e008cc0df92f3d692024c7794aa494b64d2d 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 


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

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


Testing
---


Thanks,

Sergio Pena

Review Request 68926: SENTRY-2372: SentryStore should not implement grantOptionCheck

[Sergio Pena via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

Make the grant option check on the SentryPolicyStoreProcessor so that it gets 
the group information from it. The SentryStore has the new method to check the 
grant option in the DB only.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c388689b63d5db7d71cd6cc47952d647bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1722109e381bc7be81a4673d12c1ac9f81195c47 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 3a68eb6bfa0224d1bce17f0bfccb008ab9b291f2 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
 a8868364437ac637ba4a9ad551408de0a9304984 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 a299e008cc0df92f3d692024c7794aa494b64d2d 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68547: SENTRY-2370: Create a cache of PathUpdates to send to NN

[Sergio Pena via Review Board]

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



The same PathImageRetriever feedback applies to PermissionImageRetriever.

Btw, while looking at the code completely, I think it makes more sense to keep 
the cache in the DBUpdateForwarder class. That class handles the seqNum and 
imgNum that can be used to make your logic of whether fetch a new snapshot or 
just return the cache. Also the code will be re-used for paths and permissions.


sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java
Lines 65 (patched)


Seems this method is used only internally on each class. Is it needed to be 
part of the interface and public?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 131 (original), 131 (patched)


Does this 'Clear cache if present' help for supportability? How do you know 
the cache was free or not?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 132 (patched)


Leave a comment on this call why you're releasing the cache from memory, 
and why at this point of the call.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 44 (patched)


Code convention: Need a space between the variable type and name.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 62 (patched)


Code convention: if () instead of if()



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 65 (patched)


Could you call clearCache() before creating another SoftReference? Just to 
make sure the cache is cleared and can be claimed by GC. We want to make sure 
there are not hard references leaked that causes a memory leak.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 79 (patched)


Code convention: If () instead of if()



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 80 (patched)


The SoftReference object has a clear(). I think we should call it too 
before setting the variable to null.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 97-107 (patched)


Why do we need the sequence# and image# here? Those are already checked in 
the DbUpdateForward class. I think the cache invalidation here should be done 
by looking at the current snapshot ID on the SentryStore and comparing it with 
the one store in the cache.


- Sergio Pena


On Oct. 2, 2018, 9:57 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68547/
> ---
> 
> (Updated Oct. 2, 2018, 9:57 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2370
> https://issues.apache.org/jira/browse/SENTRY-2370
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When NN requests path updates from sentry and if it exceeds the time 
> threshold, on consecutive attempts sentry will attempt to fetch the full 
> update from scratch. Instead it should cache it and update the cache before 
> sending it to NN
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java
>  11b75411d 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  3532ef33d 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
>  443434127 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  f86ce6f83 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  fb42b279a 
> 
> 
> Diff: https://reviews.apache.org/r/68547/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not allow to change the table if using only the table name

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Oct. 1, 2018, 9:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> ---
> 
> (Updated Oct. 1, 2018, 9:23 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add code to get table name and DB name based on token children size
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  6731d1a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  385ca98 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  55a79ee 
> 
> 
> Diff: https://reviews.apache.org/r/68779/diff/2/
> 
> 
> Testing
> ---
> 
> unit tests in TestOwnerPrivileges passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68547: SENTRY-2370: Create a cache of PathUpdates to send to NN

[Sergio Pena via Review Board]

> On Sept. 28, 2018, 4:56 p.m., Sergio Pena wrote:
> > What if NN does not request full images anymore? How can the cache be 
> > released from memory to avoid unused cache for the rest of the process life?
> 
> Arjun Mishra wrote:
> Sergio, the cache is releases as soon as Deltas are being sent to NN

what if there are no Deltas available to send in the next NN requests? The 
cache will live on memory until then.
Look for Java soft or weak references (and the use of SoftReferences or 
WeakReferences). These objects might help on letting the GC to clean your cache 
if it is not needed anymore.


> On Sept. 28, 2018, 4:56 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Line 158 (original), 168-172 (patched)
> > 
> >
> > I don't think this logic whether use a cache or not should be in this 
> > clas. The retriever should call the cache instead (or call the SentryStore 
> > if it needs to invalidate the current cache image).
> 
> Arjun Mishra wrote:
> This is because the logic to invalidate cache is dependent on whether 
> delta updates are being sent or not. Since the decisision to send delta 
> updates is done in DBUpdateForwarder this logic sits in this piece of code

Do you need the delta number to invalidate metadata for a snapshot? The 
PathImageRetriever can keep a cache of the current snapshot and invalidate its 
cache if the next snapshot ID is different, otherwise return the full path 
image from the cache (read a SoftReference example on how help clean the cache 
if it is not used).


> On Sept. 28, 2018, 4:56 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
> > Lines 136-137 (patched)
> > 
> >
> > Why is the cache needed to be instantiated and passed as aparameter 
> > instead of instatiating the cache inside the PathImageRetriever and 
> > PermImageRetriever instead?
> > 
> > A retriever returns the paths, but if they're cache, then it returns 
> > the ones from the cache.
> 
> Arjun Mishra wrote:
> Sergio we haev 2 path classes PathImageRetriever, PathDeltaRetriever. 
> Even though we need cache to be in PathImageRetriever, we need the cache to 
> be visible to PathDeltaRetriever so it can be invalidated.

Is a cache needed for deltas as well? I think the cache should be handled 
internally on each retriever.


- Sergio


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


On Sept. 28, 2018, 7:56 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68547/
> ---
> 
> (Updated Sept. 28, 2018, 7:56 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2370
> https://issues.apache.org/jira/browse/SENTRY-2370
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When NN requests path updates from sentry and if it exceeds the time 
> threshold, on consecutive attempts sentry will attempt to fetch the full 
> update from scratch. Instead it should cache it and update the cache before 
> sending it to NN
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathsUpdateImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermissionsUpdateImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  b8f5ce7db 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  f86ce6f83 
> 
> 
> Diff: https://reviews.apache.org/r/68547/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68547: SENTRY-2370: Create a cache of PathUpdates to send to NN

[Sergio Pena via Review Board]

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



What if NN does not request full images anymore? How can the cache be released 
from memory to avoid unused cache for the rest of the process life?


sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
Lines 53-55 (patched)


Why is a configuration needed to whether invalidate the cache or not?

Cache invalidation should be done by a certain condition that happens on 
the server. For instance, if the snapshot ID read from the DB is different from 
the cache, then it needs to invalidate the whole snapshot in the cache, or if 
the latest notification in the DB is newer than the cache, then it invalidates.

I prefer not to add a new configuration for this.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 158 (original), 168-172 (patched)


I don't think this logic whether use a cache or not should be in this clas. 
The retriever should call the cache instead (or call the SentryStore if it 
needs to invalidate the current cache image).



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 136-137 (patched)


Why is the cache needed to be instantiated and passed as aparameter instead 
of instatiating the cache inside the PathImageRetriever and PermImageRetriever 
instead?

A retriever returns the paths, but if they're cache, then it returns the 
ones from the cache.


- Sergio Pena


On Sept. 27, 2018, 10:29 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68547/
> ---
> 
> (Updated Sept. 27, 2018, 10:29 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2370
> https://issues.apache.org/jira/browse/SENTRY-2370
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When NN requests path updates from sentry and if it exceeds the time 
> threshold, on consecutive attempts sentry will attempt to fetch the full 
> update from scratch. Instead it should cache it and update the cache before 
> sending it to NN
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  2d21411e2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathsUpdateImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermissionsUpdateImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  b8f5ce7db 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  f86ce6f83 
> 
> 
> Diff: https://reviews.apache.org/r/68547/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68727: SENTRY-2306: Reduce the HMS snapshot size by ignoring partitions at default locations

[Sergio Pena via Review Board]

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



Changes look good. But I'd like to know two things before approving it:

1. Could you verify Lina's concerns (see JIRA) about what would happen if a 
partition or table location changes in the NotificationProcessor? If a 
partition location that has the same prefix as the table changes to be out of 
the table location, would this affect the HDFS ACLs?
2. Could you run a benchmark with and without this patch to see the improvement 
in numbers?

- Sergio Pena


On Sept. 24, 2018, 10:28 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68727/
> ---
> 
> (Updated Sept. 24, 2018, 10:28 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2306
> https://issues.apache.org/jira/browse/SENTRY-2306
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Ignore the partitions entries which are located in default location as they 
> share the same permissions as the table. This will reduce the snapshot size 
> decreasing the time to persist the snapshot and also time to send the full 
> update to Namenode. This is important as the partition information has lion’s 
> share in a HMS Full snapshot.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1eda41b2b6bd940a404cc1ba09a861fe783ead04 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  0b4f4aa24bd3002c50bf4d80a6fa361f66052973 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  3e27d1bbee9bea9a60e7f7e012a367957610826c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934dfc523d14eec216df00a6b7597c66c166 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
>  589acbed12855ff09309a04c9214f8daf87ea1de 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  3d7fbe3fea333d58e46cd721d610c7d8050c9de4 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  574bc4b0432824abc81fe3c0dd30d1fe01f012e5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentryService.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2de41b6700a18a358f8d5e442104dd0585 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
>  0e82b86a727f578013a63c005aa05279790344f1 
> 
> 
> Diff: https://reviews.apache.org/r/68727/diff/4/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed and also added unit and e2e tests to 
> verify the new behavior.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

[Sergio Pena via Review Board]

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



Btw, are you going to have another JIRA for the client to call this new API?

- Sergio Pena


On Sept. 23, 2018, 8:52 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 23, 2018, 8:52 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1eda41b2b6bd940a404cc1ba09a861fe783ead04 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  0b4f4aa24bd3002c50bf4d80a6fa361f66052973 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
>  25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 
> 
> 
> Diff: https://reviews.apache.org/r/68822/diff/1/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

[Sergio Pena via Review Board]

> On Sept. 21, 2018, 5:18 p.m., kalyan kumar kalvagadda wrote:
> > Sergio,
> > 
> > Sentry server is policy store and does not need to understand the 
> > action.Let's keep it that way. Logic of validiting the actions is present 
> > in the sentry bindings. I think this logic should go there instead of 
> > SentryPolicyStoreProcessor.
> 
> Sergio Pena wrote:
> I agree, but then we'll need to have the configuration in all the 
> bindings. Hive, Impala (and next SparkSQL) will need this configuration 
> otherwise they will be able to grant those privileges that we're trying to 
> avoid. If Impala does not configure those privileges correctly ,then Hive 
> will use them.
> 
> kalyan kumar kalvagadda wrote:
> I understand that the approach I suggested would need configuration for 
> both Hive and Impala. That is fine.
> 
> All these compute engines have are same. Each have them support different 
> commands. Not all operations that Impala supports are supported by Hive and 
> Impala would like to allow set of operatins which hive may not even support. 
> It could be true other way around. I really feel each of them should have 
> seperate configuration and should be validated at the client side. Sentry 
> server is just a policy store and it may not be the right place to enforce 
> this configuration.

The configuration accepts any list of privileges. The idea behind this config 
is that admins can prevent that Sentry clients grant DB policies that are not 
authorized. Doing it on the clients will not enforce this as any user with the 
access to the Sentry API will be able to grant them. This config must be in the 
server for those reasons. It is up to the Admin to decide which privileges will 
be permitted.


- Sergio


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


On Sept. 21, 2018, 2:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 21, 2018, 2:24 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1a1dd72ef9e6dd743e166979759709b2 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9e81b278c8737ea031820e20e0cf5704 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  c23b3850f097b563912c06b29ffd26ea0709b9fd 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68823: SENTRY-2416: FullUpdateInitializer metrics are not reset for each new HMS snapshot

[Sergio Pena via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

Resets the FullUpdateInitializer metrics counters to zero if another snapshot 
process starts. This will help to track how many objects are being requested 
from HMS.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 3e27d1bbee9bea9a60e7f7e012a367957610826c 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
 589acbed12855ff09309a04c9214f8daf87ea1de 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

[Sergio Pena via Review Board]

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

(Updated Sept. 21, 2018, 2:24 p.m.)


Review request for sentry.


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


Repository: sentry


Description
---

Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
comma-separated list of privileges that can be granted by Sentry DB clients. 
If the value is empty, then any privilege can be granted as it works normally 
(this is the default behavior).


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 adc1947a1a1dd72ef9e6dd743e166979759709b2 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 8cdbde4f9e81b278c8737ea031820e20e0cf5704 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 3a9623b46f7c4335db18113574170f761da9a4ca 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 c23b3850f097b563912c06b29ffd26ea0709b9fd 


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

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


Testing
---

Unit tests added.


Thanks,

Sergio Pena

Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not allow to change the table if using only the table name

[Sergio Pena via Review Board]

> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 725 (patched)
> > 
> >
> > This line fails immediatly, but instead of throwing a test case error, 
> > it goes to the finally() clause and finish with the test successfully.
> 
> Na Li wrote:
> why do you think this fails? It should work.

It failed to me while running the debug. The test does not fail, but the line 
jumps directly to the finally() clause.


- Sergio


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


On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> ---
> 
> (Updated Sept. 20, 2018, 4:24 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add code to get table name and DB name based on token children size
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  6731d1a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  55a79ee 
> 
> 
> Diff: https://reviews.apache.org/r/68779/diff/1/
> 
> 
> Testing
> ---
> 
> unit tests in TestOwnerPrivileges passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

[Sergio Pena via Review Board]

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

Review request for sentry.


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


Repository: sentry


Description
---

Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
comma-separated list of privileges that can be granted by Sentry DB clients. 
If the value is empty, then any privilege can be granted as it works normally 
(this is the default behavior).


Diffs
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 adc1947a1 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 8cdbde4f9 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 3a9623b46 


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


Testing
---

Unit tests added.


Thanks,

Sergio Pena

Re: Review Request 68727: SENTRY-2306: Reduce the HMS snapshot size by ignoring partitions at default locations

[Sergio Pena via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 4911 (patched)


This method is already public, so you don't need the @VisibleForTesting 
annotation.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
Lines 804-806 (patched)


Would be a good idea to specify which tables contain HMS Metadata as this 
interface is shared by other implementations and would need help to understand 
what to delete.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
Lines 549-555 (patched)


You could save time and code by checking if the partition location is part 
of the table location inside the PartitionTask.doTask() method instead. 

There is one part of the code there:

String partPath = pathFromURI(part.getSd().getLocation());
If (partPath != null) {
  partitionNames.add(partPath.intern());
}

If as part of the if() condition you also call 
!isPartitionLocatedWithinTableLocation(), then that should make the rest of the 
code work without too many changes.


- Sergio Pena


On Sept. 17, 2018, 3:07 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68727/
> ---
> 
> (Updated Sept. 17, 2018, 3:07 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2306
> https://issues.apache.org/jira/browse/SENTRY-2306
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Ignore the partitions entries which are located in default location as they 
> share the same permissions as the table. This will reduce the snapshot size 
> decreasing the time to persist the snapshot and also time to send the full 
> update to Namenode. This is important as the partition information has lion’s 
> share in a HMS Full snapshot.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1eda41b2b6bd940a404cc1ba09a861fe783ead04 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  0b4f4aa24bd3002c50bf4d80a6fa361f66052973 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  3e27d1bbee9bea9a60e7f7e012a367957610826c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934dfc523d14eec216df00a6b7597c66c166 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
>  589acbed12855ff09309a04c9214f8daf87ea1de 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  574bc4b0432824abc81fe3c0dd30d1fe01f012e5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentryService.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2de41b6700a18a358f8d5e442104dd0585 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
>  0e82b86a727f578013a63c005aa05279790344f1 
> 
> 
> Diff: https://reviews.apache.org/r/68727/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed and also added unit and e2e tests to 
> verify the new behavior.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not allow to change the table if using only the table name

[Sergio Pena via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 288-300 (patched)


Is it possible to reuse the extractDatabase() and extractTable() commands 
that already does most of this split of db and table names?



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


Do you need to insert data for this test case? I don't think is necessary. 
Inserts are expensive in Hive, so removing it will save time on the test 
execution.



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


Code convention: separate if and ( by a space.

Btw, I noticed that this test case has two different test paths depending 
on the value of this flag. I don't see this a good practice when writing test 
cases because if you change the flag for an unknown reason, then you will never 
execute the other test case and you won't know if it fails.

I'd rather write another test case for the grant option enabled instead of 
putting logic conditions in test cases.



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


This test case is a negative test case and it can be put in different 
method.



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


How do we validate that the error was not authorized and it was not a 
syntax error?



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


This line fails immediatly, but instead of throwing a test case error, it 
goes to the finally() clause and finish with the test successfully.



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


Aren't we running this test case in other parts of this class? Seems 
redundant to test that the ALTER TABLE fails with a table that does not exist. 

I prefer to see different methods for each test case you're trying to 
validate instead of putting several into one method.


- Sergio Pena


On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> ---
> 
> (Updated Sept. 20, 2018, 4:24 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add code to get table name and DB name based on token children size
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  6731d1a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  55a79ee 
> 
> 
> Diff: https://reviews.apache.org/r/68779/diff/1/
> 
> 
> Testing
> ---
> 
> unit tests in TestOwnerPrivileges passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68753: SENTRY-2407: SentrySchemaInfo and SQL scripts do not have the new 2.2.0 version

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Sept. 18, 2018, 9:45 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68753/
> ---
> 
> (Updated Sept. 18, 2018, 9:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2407
> https://issues.apache.org/jira/browse/sentry-2407
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> increase schema version from 2.1.0 to 2.2.0
> 
> 
> Diffs
> -
> 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.2.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.2.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.2.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.2.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.2.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-2.1.0-to-2.2.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-2.1.0-to-2.2.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-2.1.0-to-2.2.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-2.1.0-to-2.2.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-2.1.0-to-2.2.0.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.db2 
> 081f575 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.derby 
> a2cc2b6 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.mysql 
> a2cc2b6 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.oracle 
> a2cc2b6 
>   
> sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.postgres 
> a2cc2b6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  f710253 
> 
> 
> Diff: https://reviews.apache.org/r/68753/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68716: SENTRY-2403: Incorrect naming in RollingFileWithoutDeleteAppender

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Sept. 14, 2018, 1:22 p.m., Peter Somogyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68716/
> ---
> 
> (Updated Sept. 14, 2018, 1:22 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-2403
> https://issues.apache.org/jira/browse/SENTRY-2403
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> RollingFileWithoutDeleteAppender incorrectly append timestamp to logfiles 
> without removing previous one.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/appender/RollingFileWithoutDeleteAppender.java
>  fd133f34f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/appender/TestRollingFileWithoutDeleteAppender.java
>  ca9062bca 
> 
> 
> Diff: https://reviews.apache.org/r/68716/diff/1/
> 
> 
> Testing
> ---
> 
> Modified TestRollingFileWithoutDeleteAppender to check the pattern correctly.
> 
> 
> Thanks,
> 
> Peter Somogyi
> 
>

Re: Review Request 68721: SENTRY-2388: Update the pom file in master branch after release 2.1.0

[Sergio Pena via Review Board]

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


Ship it!




Looks good, can you just update the title to be 'Preparing for 2.2.0-SNAPSHOT 
release'

- Sergio Pena


On Sept. 14, 2018, 6:56 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68721/
> ---
> 
> (Updated Sept. 14, 2018, 6:56 p.m.)
> 
> 
> Review request for sentry, Curtis Sumner Sean Lee, kalyan kumar kalvagadda, 
> and Sergio Pena.
> 
> 
> Bugs: sentry-2388
> https://issues.apache.org/jira/browse/sentry-2388
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> change the version from "2.1.0-SNAPSHOT" to "2.2.0-SNAPSHOT"
> 
> 
> Diffs
> -
> 
>   pom.xml 984e15a 
>   sentry-binding/pom.xml fd5e28b 
>   sentry-binding/sentry-binding-hbase-indexer/pom.xml d50acfe 
>   sentry-binding/sentry-binding-hive-common/pom.xml e154cde 
>   sentry-binding/sentry-binding-hive-conf/pom.xml 3e7e70a 
>   sentry-binding/sentry-binding-hive-follower/pom.xml 5f8a5af 
>   sentry-binding/sentry-binding-hive/pom.xml 09d75f7 
>   sentry-binding/sentry-binding-kafka/pom.xml e4fdddf 
>   sentry-binding/sentry-binding-solr/pom.xml f086699 
>   sentry-binding/sentry-binding-sqoop/pom.xml 82cd4a6 
>   sentry-core/pom.xml 173d203 
>   sentry-core/sentry-core-common/pom.xml 75ce574 
>   sentry-core/sentry-core-model-db/pom.xml 30519aa 
>   sentry-core/sentry-core-model-indexer/pom.xml f65e49a 
>   sentry-core/sentry-core-model-kafka/pom.xml cfe9221 
>   sentry-core/sentry-core-model-solr/pom.xml 95ea02b 
>   sentry-core/sentry-core-model-sqoop/pom.xml 5629028 
>   sentry-dist/pom.xml b892536 
>   sentry-hdfs/pom.xml a015e11 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 2bfb76d 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml 1e04bf2 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/pom.xml e1bd8c3 
>   sentry-hdfs/sentry-hdfs-service/pom.xml 888f61e 
>   sentry-policy/pom.xml 1124829 
>   sentry-policy/sentry-policy-common/pom.xml 902f3e6 
>   sentry-policy/sentry-policy-engine/pom.xml 5bdd10b 
>   sentry-provider/pom.xml b075f49 
>   sentry-provider/sentry-provider-cache/pom.xml e278878 
>   sentry-provider/sentry-provider-common/pom.xml f269873 
>   sentry-provider/sentry-provider-db/pom.xml ab586f7 
>   sentry-provider/sentry-provider-file/pom.xml 6623ffe 
>   sentry-service/pom.xml b63467b 
>   sentry-service/sentry-service-api/pom.xml be95976 
>   sentry-service/sentry-service-client/pom.xml a1ae8c8 
>   sentry-service/sentry-service-server/pom.xml 4919183 
>   sentry-solr/pom.xml df8c397 
>   sentry-solr/solr-sentry-handlers/pom.xml accd581 
>   sentry-spi/pom.xml f9dbc7f 
>   sentry-tests/pom.xml e17f2a8 
>   sentry-tests/sentry-tests-hive/pom.xml 388023c 
>   sentry-tests/sentry-tests-kafka/pom.xml 03bc574 
>   sentry-tests/sentry-tests-solr/pom.xml cc0969a 
>   sentry-tests/sentry-tests-sqoop/pom.xml 13f8eed 
>   sentry-thirdparty/pom.xml bfb5fa9 
>   sentry-thirdparty/sentry-shaded/pom.xml beb89b0 
>   sentry-tools/pom.xml 05cd75c 
> 
> 
> Diff: https://reviews.apache.org/r/68721/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[Sergio Pena via Review Board]

> On Sept. 12, 2018, 1:57 p.m., kalyan kumar kalvagadda wrote:
> > I'm reviewing the code. Just thinking if this logic can be simplified. I 
> > will put more comments later today.

What's wrong with my logic? The code works, all test cases are working too.


- Sergio


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


On Sept. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68689/
> ---
> 
> (Updated Sept. 11, 2018, 8:56 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2398
> https://issues.apache.org/jira/browse/sentry-2398
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
> where the order of the upgrades found in the upgrade.order. file is not 
> the correct order.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  0d4f26c6d47991313794d250e5c232262c7559b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68689/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[Sergio Pena via Review Board]

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

(Updated Sept. 11, 2018, 8:56 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


Changes
---

Add more test cases (addressed Lina's comments)


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


Repository: sentry


Description
---

Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
where the order of the upgrades found in the upgrade.order. file is not the 
correct order.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
 0d4f26c6d47991313794d250e5c232262c7559b5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[Sergio Pena via Review Board]

> On Sept. 11, 2018, 8:22 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
> > Lines 31 (patched)
> > 
> >
> > Can you add a testing case for following upgrade order
> > 1.7.0-to-1.8.0,
> > 1.8.0-to-1.9.0,
> > 1.8.0-to-2.0.0,
> > 2.0.0-to-2.1.0,
> > 1.9.0-to-2.1.0
> > 
> > from 1.7.0 to 2.1.0 has two pathes, and you may get one of them. Using 
> > visited list to control which one to return in search function

What's the difference with the other test cases in 
testGetUpgradePathUsingMultipleSourceTargetVersions?


> On Sept. 11, 2018, 8:22 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
> > Lines 168 (patched)
> > 
> >
> > from multiVersionsOrder, you have two paths from 1.7.0 to 2.0.0. 
> > returning path of 1.7.0 to 2.0.0 directly should also be considered as 
> > correct in test.

Yes, but that will not be returned. The getUpgradePath() returns the first path 
found. There is no way to validate the other path.


- Sergio


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


On Sept. 11, 2018, 5:08 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68689/
> ---
> 
> (Updated Sept. 11, 2018, 5:08 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2398
> https://issues.apache.org/jira/browse/sentry-2398
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
> where the order of the upgrades found in the upgrade.order. file is not 
> the correct order.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  0d4f26c6d47991313794d250e5c232262c7559b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68689/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[Sergio Pena via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
where the order of the upgrades found in the upgrade.order. file is not the 
correct order.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
 0d4f26c6d47991313794d250e5c232262c7559b5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
 PRE-CREATION 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68669: SENTRY-2395: ALTER VIEW AS SELECT is asking for CREATE privileges instead of ALTER

[Sergio Pena via Review Board]

> On Sept. 7, 2018, 5:51 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 152 (patched)
> > 
> >
> > I don't think this logic is applicable for all the above cases. We 
> > should consider limiting this code to cases that are relavent.

Then I will need to repeat code on the TOK_ALTERVIEW case. The 
isAlterViewAsOperation() checks the tree is correct the same way it does it in 
this switch. The logic is the same.


> On Sept. 7, 2018, 5:51 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 302 (patched)
> > 
> >
> > I'm consufed. This comment says ALTER VIEW AS SELECT to CREATEVIEW but 
> > the API "isAlterViewAsOperation" is not invoked for case where 
> > ast.getToken().getType() is equals to HiveParser.TOK_CREATEVIEW.
> > 
> > Could you clarify?

The isAlterViewAs is set on the preAnalyze where we can check if the tree has 
such operation. The postAnalyze uses CREATEVIEW which the only way to know it 
as an ALTERVIEW_AS was by setting the boolean value in the preAnalyze.


- Sergio


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


On Sept. 7, 2018, 5:01 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68669/
> ---
> 
> (Updated Sept. 7, 2018, 5:01 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-2395
> https://issues.apache.org/jira/browse/SENTRY-2395
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch checks in the pre-analyze if the operation will be an ALTER VIEW 
> AS SELECT, and it then switches the CREATEVIEW for ALTERVIEW_AS in the 
> post-analyze.
> It also adds the new required privilege for the ALTERVIEW_AS operation.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  0ab636a97cf158cf0a219f1b0f1da35b332441a4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  0973e9832b334b83f4acb3013dc35583e5c0173a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  feb77ad6e925b5df3e06cbadbda260e01e5e94c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestOperationsPart1.java
>  42971d846f9344282f434b9cb720c7c74c592135 
> 
> 
> Diff: https://reviews.apache.org/r/68669/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68669: SENTRY-2395: ALTER VIEW AS SELECT is asking for CREATE privileges instead of ALTER

[Sergio Pena via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 152 (patched)


Then I will need to repeat code on the TOK_ALTERVIEW case. The 
isAlterViewAsOperation() checks the tree is correct the same way it does it in 
this switch. The logic is the same.



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 302 (patched)


The isAlterViewAs is set on the preAnalyze where we can check if the tree 
has such operation. The postAnalyze uses CREATEVIEW which the only way to know 
it as an ALTERVIEW_AS was by setting the boolean value in the preAnalyze.


- Sergio Pena


On Sept. 7, 2018, 5:01 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68669/
> ---
> 
> (Updated Sept. 7, 2018, 5:01 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-2395
> https://issues.apache.org/jira/browse/SENTRY-2395
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch checks in the pre-analyze if the operation will be an ALTER VIEW 
> AS SELECT, and it then switches the CREATEVIEW for ALTERVIEW_AS in the 
> post-analyze.
> It also adds the new required privilege for the ALTERVIEW_AS operation.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  0ab636a97cf158cf0a219f1b0f1da35b332441a4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  0973e9832b334b83f4acb3013dc35583e5c0173a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  feb77ad6e925b5df3e06cbadbda260e01e5e94c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestOperationsPart1.java
>  42971d846f9344282f434b9cb720c7c74c592135 
> 
> 
> Diff: https://reviews.apache.org/r/68669/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68669: SENTRY-2395: ALTER VIEW AS SELECT is asking for CREATE privileges instead of ALTER

[Sergio Pena via Review Board]

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

Review request for sentry.


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


Repository: sentry


Description
---

This patch checks in the pre-analyze if the operation will be an ALTER VIEW AS 
SELECT, and it then switches the CREATEVIEW for ALTERVIEW_AS in the 
post-analyze.
It also adds the new required privilege for the ALTERVIEW_AS operation.


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 0ab636a97cf158cf0a219f1b0f1da35b332441a4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 0973e9832b334b83f4acb3013dc35583e5c0173a 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
 feb77ad6e925b5df3e06cbadbda260e01e5e94c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestOperationsPart1.java
 42971d846f9344282f434b9cb720c7c74c592135 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68661: SENTRY-2394: Fixed typo in sentry-site.xml.service.template

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Sept. 6, 2018, 10:02 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68661/
> ---
> 
> (Updated Sept. 6, 2018, 10:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/SENTRY-2394
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/SENTRY-2394
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> [SENTRY-2394](https://issues.apache.org/jira/browse/SENTRY-2394): Fixed typo 
> in sentry-site.xml.service.template
> 
> 
> Diffs
> -
> 
>   conf/sentry-site.xml.service.template 
> e4916600a9a8f182cf50ac57a19099224926f93b 
> 
> 
> Diff: https://reviews.apache.org/r/68661/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 68657: SENTRY-2392: Add metrics statistics to list_user_privileges and list_role_privileges API

[Sergio Pena via Review Board]

> On Sept. 6, 2018, 5:56 p.m., Steve Moist wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Line 1397 (original), 1397 (patched)
> > 
> >
> > Shouldn't there be a finally block here to stop the timer?

It is using try-with-resources which closes the object inside the try() 
sentence.


- Sergio


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


On Sept. 6, 2018, 2:57 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68657/
> ---
> 
> (Updated Sept. 6, 2018, 2:57 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2392
> https://issues.apache.org/jira/browse/sentry-2392
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add some metric statistics to the new SentryPolicyStoreProcessor API that 
> returns a list of users and roles privileges. This would be good to collect 
> in order to understand how this API is used.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  35c7d079be3de8d0cf7e8067d4b5ef46c10f8d72 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  008a48efc0b1a2a8703a301c85cf0068dd171f38 
> 
> 
> Diff: https://reviews.apache.org/r/68657/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68657: SENTRY-2392: Add metrics statistics to list_user_privileges and list_role_privileges API

[Sergio Pena via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

Add some metric statistics to the new SentryPolicyStoreProcessor API that 
returns a list of users and roles privileges. This would be good to collect in 
order to understand how this API is used.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 35c7d079be3de8d0cf7e8067d4b5ef46c10f8d72 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 008a48efc0b1a2a8703a301c85cf0068dd171f38 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68624: SENTRY-2315: The grant all operation is not dropping the create/alter/drop/index/lock privileges.

[Sergio Pena via Review Board]

> On Sept. 5, 2018, 4:28 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 927 (patched)
> > 
> >
> > Can you add the test cases that 
> > 1) user already has "ALL with grant option", and then grant with "ALL", 
> > what happens?
> > 2) user already has "ALL", and then grant with "ALL with grant option", 
> > what happens?

Done, I added the new tests. They were no errors.


- Sergio


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


On Sept. 5, 2018, 4:57 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68624/
> ---
> 
> (Updated Sept. 5, 2018, 4:57 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2315
> https://issues.apache.org/jira/browse/sentry-2315
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Fixes the issue where GRANT ALL does not include the ALTER privilege. 
> 
> It does not fix the REVOKE command where revoking a single privilege will end 
> up revoking ALL and granting all invididual supported privileges. For now, 
> such behavior only works for SELECT and INSERT.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  db8c08ba0839400c65658e90ebd18906fed9919f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  6b732ea5a89dafd94a6a3ae9c423747118352d84 
> 
> 
> Diff: https://reviews.apache.org/r/68624/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68624: SENTRY-2315: The grant all operation is not dropping the create/alter/drop/index/lock privileges.

[Sergio Pena via Review Board]

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

(Updated Sept. 5, 2018, 4:57 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Changes
---

Added test cases to verify that grant all replaces grant all with grant and 
viceversa.


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


Repository: sentry


Description
---

Fixes the issue where GRANT ALL does not include the ALTER privilege. 

It does not fix the REVOKE command where revoking a single privilege will end 
up revoking ALL and granting all invididual supported privileges. For now, such 
behavior only works for SELECT and INSERT.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 db8c08ba0839400c65658e90ebd18906fed9919f 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 6b732ea5a89dafd94a6a3ae9c423747118352d84 


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

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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68624: SENTRY-2315: The grant all operation is not dropping the create/alter/drop/index/lock privileges.

[Sergio Pena via Review Board]

> On Sept. 4, 2018, 9:49 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 866 (patched)
> > 
> >
> > if action is ALL or ACTION_ALL, SHOULD you continue? Before calling 
> > this function, if the action to grant is all, and all is already granted, 
> > we can do nothing
> 
> Sergio Pena wrote:
> No. This part of the code drops the privileges before granting the new 
> one. If I leave ALL, then we might have two entries with ALL. OWNER is the 
> special privilege that cannot be combined with ALL because means the same.
> 
> Na Li wrote:
> 1) If the principle has "ALL with grant option", and to be granted with 
> "ALL". If you remove existing one, then grant option is lost. Is this the 
> correct behavior?
> 2) If the principle has "ALL", and to be granted with "ALL with grant 
> option". If you remove existing one, then ALL with grant option is granted. I 
> think this is the correct behavior.

I don't think so. The code that is executed after all privileges are dropped is 
this:

mPrivilege = getMSentryPrivilege(privilege, pm);
if (mPrivilege == null) {
  mPrivilege = convertToMSentryPrivilege(privilege);
}
mPrivilege.appendPrincipal(mPrincipal);
pm.makePersistent(mPrivilege);

The above code will get the privilege to be granted, if it does not exist 
(meaning if the all was dropped), then it will create a new ALL privilege with 
the grant option.


- Sergio


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


On Sept. 4, 2018, 7:02 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68624/
> ---
> 
> (Updated Sept. 4, 2018, 7:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2315
> https://issues.apache.org/jira/browse/sentry-2315
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Fixes the issue where GRANT ALL does not include the ALTER privilege. 
> 
> It does not fix the REVOKE command where revoking a single privilege will end 
> up revoking ALL and granting all invididual supported privileges. For now, 
> such behavior only works for SELECT and INSERT.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  db8c08ba0839400c65658e90ebd18906fed9919f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  6b732ea5a89dafd94a6a3ae9c423747118352d84 
> 
> 
> Diff: https://reviews.apache.org/r/68624/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68623: SENTRY-2376: Bump Jackson libraries versions to 1.9.13 and 2.9.6

[Sergio Pena via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

Bump Jackson version and exclude jackson-xc and jackson-annotations libraries 
to avoid mixing older versions.


Diffs
-

  pom.xml fb9950d0feca18f7f49531c6c38bb7357b565e5b 
  sentry-tests/sentry-tests-hive/pom.xml 
a167e76ba11692351e1150c13efbf48f25530cad 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68487: SENTRY-2366: Exclude jackson transitive dependencies

[Sergio Pena via Review Board]

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

(Updated Aug. 23, 2018, 7:20 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

Sentry uses Jackson 1.8.8, but there are other versions of jackson jars that 
are brought by transitive dependencies. Even if this is not conflicting, we 
should harmonize the version that Sentry should use for Jackson.


Diffs (updated)
-

  pom.xml dcf107680c3b395819043136bcc6e179a60ada35 
  sentry-provider/sentry-provider-db/pom.xml 
bfe91d7e1f21b7fee73a9ccb80229eccae4531aa 
  sentry-service/sentry-service-server/pom.xml 
c835292790dbc31b164dc1668c860a3f58aeda6b 
  sentry-tests/sentry-tests-hive/pom.xml 
c753acf31580d9ff553da35076a523552b54b244 


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

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


Testing
---


Thanks,

Sergio Pena

Review Request 68487: SENTRY-2366: Exclude jackson transitive dependencies

[Sergio Pena via Review Board]

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

Review request for sentry.


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


Repository: sentry


Description
---

Sentry uses Jackson 1.8.8, but there are other versions of jackson jars that 
are brought by transitive dependencies. Even if this is not conflicting, we 
should harmonize the version that Sentry should use for Jackson.


Diffs
-

  pom.xml dcf107680c3b395819043136bcc6e179a60ada35 
  sentry-binding/sentry-binding-hbase-indexer/pom.xml 
d50acfec954acf8e8d9682425122039e6f5a2724 
  sentry-binding/sentry-binding-hive-follower/pom.xml 
5f8a5afb479b783d027a005ea8f72590445b8386 
  sentry-binding/sentry-binding-hive/pom.xml 
09d75f7de961aa0d3d03c875fb896504c0c72a3e 
  sentry-binding/sentry-binding-solr/pom.xml 
f08669994aa6d058fc9c92e6a8b576063602cc95 
  sentry-hdfs/sentry-hdfs-common/pom.xml 
2bfb76d6686324a7a949b01bfb7557fac4c2b89d 
  sentry-provider/sentry-provider-db/pom.xml 
bfe91d7e1f21b7fee73a9ccb80229eccae4531aa 
  sentry-service/sentry-service-server/pom.xml 
c835292790dbc31b164dc1668c860a3f58aeda6b 
  sentry-solr/solr-sentry-handlers/pom.xml 
accd581263447578724b5b7d7699f9e42efed083 
  sentry-tests/sentry-tests-hive/pom.xml 
c753acf31580d9ff553da35076a523552b54b244 
  sentry-tests/sentry-tests-kafka/pom.xml 
03bc57453922d99c45f8e2bca8bdb532a6115170 
  sentry-tests/sentry-tests-solr/pom.xml 
cc0969aebadb69e08f830a5a97d95962a3258da3 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68444: SENTRY-2318: Sentry listener should log the failure if grant/revoke of owner privilege fails

[Sergio Pena via Review Board]

> On Aug. 21, 2018, 7:52 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 265-268 (original), 265-268 (patched)
> > 
> >
> > Agree with Lina. We cannot throw an exception in HMS as the listener is 
> > run after HMS already committed the operation. This may cause HMS to retry 
> > the commit .
> 
> kalyan kumar kalvagadda wrote:
> Just looked at the HMS code closely. This exception will cause HMS to 
> retry. This is wrong behavior in HMS for a failure in Post Event Listener.

HMS should not throw the exception back to HS2 for post-event listeners, 
otherwise the DDL operations will be committed with errors.


- Sergio


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


On Aug. 21, 2018, 5:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68444/
> ---
> 
> (Updated Aug. 21, 2018, 5:45 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2318
> https://issues.apache.org/jira/browse/SENTRY-2318
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If the grant/revoke of owner privilege fails for any reason it should logged 
> be as an error in the hive logs otherwise this will go un-noticed.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  c37f3706092f2239e45e1cef2f31cb8ab1c886f5 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
>  98de8e36adfd790996aba8e61a164d85e80bc006 
> 
> 
> Diff: https://reviews.apache.org/r/68444/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests to cover the change.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68456: SENTRY-2360: Update the pom file in 2.0 branch

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Aug. 21, 2018, 9:33 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68456/
> ---
> 
> (Updated Aug. 21, 2018, 9:33 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2360
> https://issues.apache.org/jira/browse/SENTRY-2360
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> updated the version from 2.0 to 2.0-2-SNAPSHOT in the pom files.
> 
> 
> Diffs
> -
> 
>   pom.xml 94b952fa7a4d59e58752d575b68a5260ef8cc2ab 
>   sentry-binding/pom.xml acffc5f55085a66db82b2dacc81e42b952a531ae 
>   sentry-binding/sentry-binding-hive-common/pom.xml 
> eb310da2d9273931b1d55a4a916c7749142ecbe5 
>   sentry-binding/sentry-binding-hive-conf/pom.xml 
> 232a3f28165b0fb14f80ccc2f7fe885d1799be71 
>   sentry-binding/sentry-binding-hive-follower/pom.xml 
> e5ecdfd3acaed632434ae392206ffe0f20d602e2 
>   sentry-binding/sentry-binding-hive-v2/pom.xml 
> 18dbe6484fb833198536c955c306c32194818183 
>   sentry-binding/sentry-binding-hive/pom.xml 
> 085a12e46460154e488d0f7d2583e4155498bed5 
>   sentry-binding/sentry-binding-kafka/pom.xml 
> fe8b3bbe1fb4cbb121c5ce19059b22dbffd7d722 
>   sentry-binding/sentry-binding-solr/pom.xml 
> 8a3fd1dcce06541695b1bda9b577dd04d1a2e5b0 
>   sentry-binding/sentry-binding-sqoop/pom.xml 
> d67aa1782b67d540531aba32a9f4ab350ecd832d 
>   sentry-core/pom.xml 6c877a70cc058d75711f2c3b5dbe9bd55b0fa721 
>   sentry-core/sentry-core-common/pom.xml 
> ee91c1a042cb5845e1ea27726afd22968fd32bf1 
>   sentry-core/sentry-core-model-db/pom.xml 
> 66df5d139037bb61df07bca025677e8233227088 
>   sentry-core/sentry-core-model-indexer/pom.xml 
> f85eb3fa91204e555284e07630277de13ce07674 
>   sentry-core/sentry-core-model-kafka/pom.xml 
> fc2daeb9e233d7df92661b430f5803ba1d8991a3 
>   sentry-core/sentry-core-model-solr/pom.xml 
> bd3784b54560223535275fad2b8766bf56ffcc3e 
>   sentry-core/sentry-core-model-sqoop/pom.xml 
> 8e58a1fbd30fdbb53fb41315fb2a8bb32edfc9c9 
>   sentry-dist/pom.xml ecd0d37e9781bc5e5092f8cff7ab8efa9b76a8c5 
>   sentry-hdfs/pom.xml d8b9efb248fc0c9cae8f73a61d35d8d690b81578 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> fe8fcf0eb48985ebd6957e1b3d711e2b7a1ab5ee 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml 
> 624fccc2759737e128014620e6a6c38636fcacd2 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/pom.xml 
> f728b26e15583a789f7b6b27051cb57ae0c20933 
>   sentry-hdfs/sentry-hdfs-service/pom.xml 
> 158e837f92d81fc63e2438163a4b85eca351c926 
>   sentry-policy/pom.xml d56c836399fa147bbb048fe5dfb3e750c1b6acab 
>   sentry-policy/sentry-policy-common/pom.xml 
> 065cddeea6669453af8575eed9693866bd28b0ae 
>   sentry-policy/sentry-policy-engine/pom.xml 
> c97347790ddb70c5a75aa5d6efb972d7c830f117 
>   sentry-policy/sentry-policy-indexer/pom.xml 
> b25a0cc0d2cf8d1cf25880d1cff23cccf4ec78cf 
>   sentry-provider/pom.xml 6ce30f5b0fe3edd5d2eb33932e538a82519bebf9 
>   sentry-provider/sentry-provider-cache/pom.xml 
> dedd89444ddeea3782c2ab3eb4362ae8bebe508f 
>   sentry-provider/sentry-provider-common/pom.xml 
> 4536deab9dffee99cf4e420d13bc2ab10ce3d052 
>   sentry-provider/sentry-provider-db/pom.xml 
> 679868a689aa1bdf602a58505d53a57a9eacc953 
>   sentry-provider/sentry-provider-file/pom.xml 
> 586ab5f700c20e8445ee9c64474568674988dbb8 
>   sentry-solr/pom.xml 8060575d882926beabdc948a7e8b3ba8ebafeb81 
>   sentry-solr/solr-sentry-handlers/pom.xml 
> 7045c48a17d573eab779d6443a59b46cd16eacb8 
>   sentry-tests/pom.xml a82f2adb6b03a82a9198e0a7d36df92a9916d965 
>   sentry-tests/sentry-tests-hive-v2/pom.xml 
> ecb9dc75a4f5aa764026a7840ca05395feec1ba1 
>   sentry-tests/sentry-tests-hive/pom.xml 
> 64ab1e214e4aaac6c7d4f50d883ba7dfc71f2e2d 
>   sentry-tests/sentry-tests-kafka/pom.xml 
> 4b89a59162998a5fdbb2348adba90ec24ce752f3 
>   sentry-tests/sentry-tests-solr/pom.xml 
> 01f5c6c2992cdd03df3fac5aa7836f116b03469b 
>   sentry-tests/sentry-tests-sqoop/pom.xml 
> 2d425a42da38395366fbbd8a2762bdbfa01d32d8 
>   sentry-tools/pom.xml 64508fd35d716f3fb9b8ccb1e7bafccdd4a429e6 
> 
> 
> Diff: https://reviews.apache.org/r/68456/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68444: SENTRY-2318: Sentry listener should log the failure if grant/revoke of owner privilege fails

[Sergio Pena via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 265-268 (original), 265-268 (patched)


Agree with Lina. We cannot throw an exception in HMS as the listener is run 
after HMS already committed the operation. This may cause HMS to retry the 
commit .


- Sergio Pena


On Aug. 21, 2018, 5:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68444/
> ---
> 
> (Updated Aug. 21, 2018, 5:45 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2318
> https://issues.apache.org/jira/browse/SENTRY-2318
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If the grant/revoke of owner privilege fails for any reason it should logged 
> be as an error in the hive logs otherwise this will go un-noticed.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  c37f3706092f2239e45e1cef2f31cb8ab1c886f5 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
>  98de8e36adfd790996aba8e61a164d85e80bc006 
> 
> 
> Diff: https://reviews.apache.org/r/68444/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests to cover the change.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68433: SENTRY-2356: Sentry Server should provide API for Sentry Client to get Owner Privilege Type

[Sergio Pena via Review Board]

> On Aug. 21, 2018, 1:34 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
> > Lines 469-470 (patched)
> > 
> >
> > I think we should make a generic call to get information from Sentry. 
> > This is the first call to get such info and there will be more in the 
> > future, so we may end up having several methods for one specific type of 
> > information.
> > 
> > Hive has a similar generic API call to get information. This is called: 
> > GetInfo() and accepts a info request as a parameter that include several 
> > types of information to get. For Sentry, we can start with a 
> > CONFIG_DB_OWNER_PRIVILEGE.
> > 
> > See 
> > https://github.com/apache/hive/blob/master/service-rpc/if/TCLIService.thrift#L1258
> 
> Sergio Pena wrote:
> Perhas use the same config name as the request. 
> CONFIG_DB_POLICY_OWNER_AS_PRIVILEGE
> 
> Na Li wrote:
> We already have a general function 
> SentryPolicyServiceClient.getConfigValue(), which calls 
> SentryPolicyStoreProcessor.get_sentry_config_value. The returned value is 
> String. The new function I added is enum to avoid mistake.

I see. I didn't know we had such method. Then I don't see the reason adding a 
new API just to avoid mistakes. I prefer not to continue on this patch if this 
API already exists.


- Sergio


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


On Aug. 20, 2018, 9:44 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68433/
> ---
> 
> (Updated Aug. 20, 2018, 9:44 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2356
> https://issues.apache.org/jira/browse/sentry-2356
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add function for sentry client to get owner privilege type from sentry server
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeType.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeTypeRequest.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeTypeResponse.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
>  3ef1624 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  a2213ae 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e56 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyServiceClientDefaultImpl.java
>  e2e1e69 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  36b635a 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  2de6253 
> 
> 
> Diff: https://reviews.apache.org/r/68433/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68433: SENTRY-2356: Sentry Server should provide API for Sentry Client to get Owner Privilege Type

[Sergio Pena via Review Board]

> On Aug. 21, 2018, 1:34 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
> > Lines 469-470 (patched)
> > 
> >
> > I think we should make a generic call to get information from Sentry. 
> > This is the first call to get such info and there will be more in the 
> > future, so we may end up having several methods for one specific type of 
> > information.
> > 
> > Hive has a similar generic API call to get information. This is called: 
> > GetInfo() and accepts a info request as a parameter that include several 
> > types of information to get. For Sentry, we can start with a 
> > CONFIG_DB_OWNER_PRIVILEGE.
> > 
> > See 
> > https://github.com/apache/hive/blob/master/service-rpc/if/TCLIService.thrift#L1258

Perhas use the same config name as the request. 
CONFIG_DB_POLICY_OWNER_AS_PRIVILEGE


- Sergio


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


On Aug. 20, 2018, 9:44 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68433/
> ---
> 
> (Updated Aug. 20, 2018, 9:44 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2356
> https://issues.apache.org/jira/browse/sentry-2356
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add function for sentry client to get owner privilege type from sentry server
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeType.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeTypeRequest.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeTypeResponse.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
>  3ef1624 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  a2213ae 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e56 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyServiceClientDefaultImpl.java
>  e2e1e69 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  36b635a 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  2de6253 
> 
> 
> Diff: https://reviews.apache.org/r/68433/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68433: SENTRY-2356: Sentry Server should provide API for Sentry Client to get Owner Privilege Type

[Sergio Pena via Review Board]

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




sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 469-470 (patched)


I think we should make a generic call to get information from Sentry. This 
is the first call to get such info and there will be more in the future, so we 
may end up having several methods for one specific type of information.

Hive has a similar generic API call to get information. This is called: 
GetInfo() and accepts a info request as a parameter that include several types 
of information to get. For Sentry, we can start with a 
CONFIG_DB_OWNER_PRIVILEGE.

See 
https://github.com/apache/hive/blob/master/service-rpc/if/TCLIService.thrift#L1258


- Sergio Pena


On Aug. 20, 2018, 9:44 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68433/
> ---
> 
> (Updated Aug. 20, 2018, 9:44 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2356
> https://issues.apache.org/jira/browse/sentry-2356
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add function for sentry client to get owner privilege type from sentry server
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeType.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeTypeRequest.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryOwnerPrivilegeTypeResponse.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
>  3ef1624 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  a2213ae 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e56 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyServiceClientDefaultImpl.java
>  e2e1e69 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  36b635a 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  2de6253 
> 
> 
> Diff: https://reviews.apache.org/r/68433/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> ---
> 
> (Updated Aug. 17, 2018, 7:01 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When Solr is using Sentry server for authorization, it issues a lot of calls 
> to getGroupsByRoles() function in DelegateSentryStore.
> 
> This function isn't very efficient - it walks over each role in the set, 
> obtains role by name, get groups for each role, and collects all group names 
> into a set.
> 
> It may be possible to optimize it.
> 
> Also, in SentryGenericPolicyProcessor class method 
> list_sentry_roles_by_group() would make N transactions to build the roles to 
> set of groups map. Instead, make it to a single transaction. This will 
> significantly speed up operation
> 
> Attach one or more files to this issue
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
>  1cc4b1b37 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  3026a6225 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
>  eec2757d3 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java
>  4c207e9b4 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
>  69d16238f 
> 
> 
> Diff: https://reviews.apache.org/r/64661/diff/10/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

[Sergio Pena via Review Board]

> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 244 (patched)
> > 
> >
> > We should do something to avoid this conflict with the TSentryRole 
> > type. getTSentryRolesByGroupName() is the implementation of the 
> > SentryStoreLayer, but this returns a different type which breaks the 
> > contract of the implementation.
> > 
> > Why is the SentryStoreLayer using a different TSentryRole than the 
> > DelegationSentryStore? If we use  api.service.thrift.TSentryRole in 
> > SentryStoreLayer, then that would fix the problem.
> 
> Arjun Mishra wrote:
> It doesn't return a different type. They both return 
> org.apache.sentry.api.generic.thrift.TSentryRole
> 
> Arjun Mishra wrote:
> DelegateSentryStore calls SentryStore.getTSentryRolesByGroupName() which 
> uses org.apache.sentry.api.service.thrift.TSentryRole.
> 
> Na Li wrote:
> I agree with Arjun. 
> DelegationSentryStore.getTSentryRolesByGroupName() returns set of 
> org.apache.sentry.api.generic.thrift.TSentryRole
> SentryStore, which is called by DelegationSentryStore, returns set of 
> org.apache.sentry.api.service.thrift.TSentryRole.
> That is why DelegationSentryStore calls SentryStore and converts 
> org.apache.sentry.api.service.thrift.TSentryRole to 
> org.apache.sentry.api.generic.thrift.TSentryRole

Isn't this a performance issue as well? The SentryStore reads a set of 
MSentryRole which is converted to service.thrift.TSentryRole which is then 
converted to generic.thrift.TSentryRole. That does not look right.

Should we have another SentryStore method that returns the set of MSentryRole 
instead and just walk through it to convert it to generic.TSentryRole?


- Sergio


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


On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> ---
> 
> (Updated Aug. 17, 2018, 2:01 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When Solr is using Sentry server for authorization, it issues a lot of calls 
> to getGroupsByRoles() function in DelegateSentryStore.
> 
> This function isn't very efficient - it walks over each role in the set, 
> obtains role by name, get groups for each role, and collects all group names 
> into a set.
> 
> It may be possible to optimize it.
> 
> Also, in SentryGenericPolicyProcessor class method 
> list_sentry_roles_by_group() would make N transactions to build the roles to 
> set of groups map. Instead, make it to a single transaction. This will 
> significantly speed up operation
> 
> Attach one or more files to this issue
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
>  1cc4b1b37 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  3026a6225 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
>  eec2757d3 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java
>  4c207e9b4 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
>  69d16238f 
> 
> 
> Diff: https://reviews.apache.org/r/64661/diff/9/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

[Sergio Pena via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 244 (patched)


We should do something to avoid this conflict with the TSentryRole type. 
getTSentryRolesByGroupName() is the implementation of the SentryStoreLayer, but 
this returns a different type which breaks the contract of the implementation.

Why is the SentryStoreLayer using a different TSentryRole than the 
DelegationSentryStore? If we use  api.service.thrift.TSentryRole in 
SentryStoreLayer, then that would fix the problem.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 250-258 (patched)


This method returns a set of TSentryRole, isn't the 
getTSentryRolesByGroupName() call enough? Why is it neede to walk through the 
Set returned by the call, and create another set of TSentryRole?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 266 (patched)


This will throw a NullPointerException if roles is null. Is it necessary to 
throw an exception? Can we return an emptySet instead?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
Lines 23 (patched)


If this uses the service.thrift then the DelegationSentrySTore won't have 
the issue with the types.


- Sergio Pena


On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> ---
> 
> (Updated Aug. 17, 2018, 2:01 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When Solr is using Sentry server for authorization, it issues a lot of calls 
> to getGroupsByRoles() function in DelegateSentryStore.
> 
> This function isn't very efficient - it walks over each role in the set, 
> obtains role by name, get groups for each role, and collects all group names 
> into a set.
> 
> It may be possible to optimize it.
> 
> Also, in SentryGenericPolicyProcessor class method 
> list_sentry_roles_by_group() would make N transactions to build the roles to 
> set of groups map. Instead, make it to a single transaction. This will 
> significantly speed up operation
> 
> Attach one or more files to this issue
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
>  1cc4b1b37 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  3026a6225 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
>  eec2757d3 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java
>  4c207e9b4 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
>  69d16238f 
> 
> 
> Diff: https://reviews.apache.org/r/64661/diff/9/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68394: SENTRY-2355: Merge the DB owner privileges configurations into one enum configuration

[Sergio Pena via Review Board]

> On Aug. 17, 2018, 4:57 a.m., Na Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
> > Line 890 (original), 894 (patched)
> > 
> >
> > should you set it to be Null when OwnerPrivilegeEnabled == false since 
> > "SentryOwnerPrivilegeType SENTRY_DB_POLICY_STORE_OWNER_AS_PRIVILEGE_DEFAULT 
> > = SentryOwnerPrivilegeType.ALL;"?

Done. I changed the default to be NONE to avoid incompatibilities between 2.0 
and 2.1. I also set the value to NONE if the OwnerPrivilegeEnabled is false.


- Sergio


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


On Aug. 17, 2018, 2:47 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68394/
> ---
> 
> (Updated Aug. 17, 2018, 2:47 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2355
> https://issues.apache.org/jira/browse/sentry-2355
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch replaces the owner configurations:
> - sentry.enable.owner.privileges = [true,false]
> - sentry.enable.owner.privileges.with.grant = [true,false]
> 
> to:
> - sentry.db.policy.store.owner.as.privilege = [none,all,all_with_grant]
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  9a2091ae195bea3dc1ef8f966a598459e8446d7c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  07221afaa2c3de3c723c7282da593e55353e76be 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  8b32e7b2f7b12cf2aa60eb955f72db85afbe41cd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  94dbd70c002de871dbc53d463d537798733ef99b 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  becdc52631d348fe4ddc650dab64abd1370c2469 
> 
> 
> Diff: https://reviews.apache.org/r/68394/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68394: SENTRY-2355: Merge the DB owner privileges configurations into one enum configuration

[Sergio Pena via Review Board]

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

(Updated Aug. 17, 2018, 2:47 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

This patch replaces the owner configurations:
- sentry.enable.owner.privileges = [true,false]
- sentry.enable.owner.privileges.with.grant = [true,false]

to:
- sentry.db.policy.store.owner.as.privilege = [none,all,all_with_grant]


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 9a2091ae195bea3dc1ef8f966a598459e8446d7c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 07221afaa2c3de3c723c7282da593e55353e76be 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 8b32e7b2f7b12cf2aa60eb955f72db85afbe41cd 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 94dbd70c002de871dbc53d463d537798733ef99b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 becdc52631d348fe4ddc650dab64abd1370c2469 


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

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


Testing
---


Thanks,

Sergio Pena

Review Request 68394: SENTRY-2355: Merge the DB owner privileges configurations into one enum configuration

[Sergio Pena via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

This patch replaces the owner configurations:
- sentry.enable.owner.privileges = [true,false]
- sentry.enable.owner.privileges.with.grant = [true,false]

to:
- sentry.db.policy.store.owner.as.privilege = [none,all,all_with_grant]


Diffs
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 9a2091ae195bea3dc1ef8f966a598459e8446d7c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 07221afaa2c3de3c723c7282da593e55353e76be 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 8b32e7b2f7b12cf2aa60eb955f72db85afbe41cd 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 94dbd70c002de871dbc53d463d537798733ef99b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 becdc52631d348fe4ddc650dab64abd1370c2469 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

[Sergio Pena via Review Board]

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



What's the expected performance improvement we're getting with this? Have you 
run tests to check how much time we're saving?


sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
Line 590 (original), 590 (patched)


Code convention: Need a space to separate the variable name from the type.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 244-246 (original), 249-252 (patched)


Is it necessary to throw a NullPointerException if roles is null?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 278 (patched)


Is it necessary to throw a NPE if roles is null?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 294 (patched)


You can construct the hasmap with an initial capacitity of roles.size() to 
avoid the hashmap to be resized during the for loop.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 295-297 (patched)


Code convention: Space between for and ()

Btw, this has 2 loops. I think we can avoid using a 2nd loop by making the 
query to request all roles instead of all groups. That way you just walk 
through the list of roles and put each role and its groups in the map (this 
saves one loop and the containsKey and get calls).



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
Lines 139-142 (patched)


Need to fill information of @param, @return and @throws. I usually put in 
@throws why I am expecting this exception.



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java
Lines 292 (patched)


Coding convention: space >mockMap.


- Sergio Pena


On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> ---
> 
> (Updated Aug. 16, 2018, 7:55 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When Solr is using Sentry server for authorization, it issues a lot of calls 
> to getGroupsByRoles() function in DelegateSentryStore.
> 
> This function isn't very efficient - it walks over each role in the set, 
> obtains role by name, get groups for each role, and collects all group names 
> into a set.
> 
> It may be possible to optimize it.
> 
> Also, in SentryGenericPolicyProcessor class method 
> list_sentry_roles_by_group() would make N transactions to build the roles to 
> set of groups map. Instead, make it to a single transaction. This will 
> significantly speed up operation
> 
> Attach one or more files to this issue
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
>  1cc4b1b37 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  3026a6225 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
>  eec2757d3 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java
>  4c207e9b4 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
>  69d16238f 
> 
> 
> Diff: https://reviews.apache.org/r/64661/diff/5/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68367: SENTRY-2354: Beeline error message only shows first required permission that failed access check

[Sergio Pena via Review Board]

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




sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
Lines 74-80 (patched)


Why do we need to bring this thread local here? is there another way to get 
a list of the privileges without this variable? Why is it trhead local btw?



sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
Lines 429 (patched)


Is there an example of the message that will be printed? I think it would 
be useful to get some examples and ask a technical writer feedback about it. I 
feel we'll have unorganized information in the output.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java
Lines 464 (patched)


That hard-coded value looks like one of our server names. Can we use a fake 
name?



sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java
Lines 78-79 (patched)


Is it necessary to expose this to the interface?


- Sergio Pena


On Aug. 15, 2018, 10:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68367/
> ---
> 
> (Updated Aug. 15, 2018, 10:23 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2354
> https://issues.apache.org/jira/browse/sentry-2354
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When multiple privileges are required on a Hive operation, return the 
> privilege that failed access check and the required privileges not checked 
> yet (they may fail the access check, or may not. We don't check those 
> privileges to avoid unnecessary overhead)
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  6a1556f 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java
>  3bbf6fb 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java
>  aecfe5b 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/NoAuthorizationProvider.java
>  61400ca 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  1e1aa63 
> 
> 
> Diff: https://reviews.apache.org/r/68367/diff/1/
> 
> 
> Testing
> ---
> 
> Add test case that verifies the behavior stated above. All test cases in 
> TestHiveAuthzBindings passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67888: SENTRY-2308: Create privilege on table has no use case

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 8:07 p.m., Na Li wrote:
> > can you add test case for this change?

The new patch has a new test case for this.


- Sergio


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


On Aug. 16, 2018, 4:18 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67888/
> ---
> 
> (Updated Aug. 16, 2018, 4:18 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: sentry-2308
> https://issues.apache.org/jira/browse/sentry-2308
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not allow CREATE privileges on a table. There is no use case for it.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java
>  2c662e55382adc36abd18b6b6b82aa7a0b2f210b 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  beca2f8f3fd03953d5652337dee7e69313f098a3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestAllowedGrantPrivileges.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestDescribeMetadataPrivileges.java
>  54f4f2fdba964ebd1e610a87dfaa370721741af2 
> 
> 
> Diff: https://reviews.apache.org/r/67888/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 67888: SENTRY-2308: Create privilege on table has no use case

[Sergio Pena via Review Board]

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

(Updated Aug. 16, 2018, 4:18 p.m.)


Review request for sentry and Arjun Mishra.


Changes
---

Added a new test case TestAllowedGrantPrivileges


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


Repository: sentry


Description
---

Do not allow CREATE privileges on a table. There is no use case for it.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java
 2c662e55382adc36abd18b6b6b82aa7a0b2f210b 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
 beca2f8f3fd03953d5652337dee7e69313f098a3 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestAllowedGrantPrivileges.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestDescribeMetadataPrivileges.java
 54f4f2fdba964ebd1e610a87dfaa370721741af2 


Diff: https://reviews.apache.org/r/67888/diff/3/

Changes: https://reviews.apache.org/r/67888/diff/2-3/


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68332: SENTRY-2352: User roles with ALTER on a table can not show or describe the table on which they have ALTER

[Sergio Pena via Review Board]

> On Aug. 13, 2018, 11:25 p.m., Na Li wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
> > Line 104 (original), 104 (patched)
> > 
> >
> > does Alter apply to column?

Yes, a user with ALTER privileges should be able to view column metadata. ALTER 
Is used to change column names, add columns ,etc; so they need to be able to 
see what they can change.


> On Aug. 13, 2018, 11:25 p.m., Na Li wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
> > Lines 177 (patched)
> > 
> >
> > should Drop be included as well?

I don't see a need for a user with DROP privileges to be able to view the 
column names and the rest of the table metadata. If a user has DROP privileges, 
then they should only see the table with the SHOW TABLES.


- Sergio


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


On Aug. 13, 2018, 10:49 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68332/
> ---
> 
> (Updated Aug. 13, 2018, 10:49 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2352
> https://issues.apache.org/jira/browse/sentry-2352
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added the following matrix of privileges on the DESCRIBE TABLE and SHOW 
> TABLES:
> 
> DESCRIBE table allowed and not allowed privileges:
> 
>   { null,  NOT_ALLOWED }, // Means no privileges
>   { DBModelAction.ALL, ALLOWED },
>   { DBModelAction.CREATE,  NOT_ALLOWED },
>   { DBModelAction.SELECT,  ALLOWED },
>   { DBModelAction.INSERT,  ALLOWED },
>   { DBModelAction.ALTER,   ALLOWED },
>   { DBModelAction.DROP,NOT_ALLOWED },
>   { DBModelAction.INDEX,   NOT_ALLOWED },
>   { DBModelAction.LOCK,NOT_ALLOWED },
>   
> SHOW TABLES allowed and not allowed privileges:
> 
>   { null,  NOT_ALLOWED }, // Means no privileges
>   { DBModelAction.ALL, ALLOWED },
>   { DBModelAction.CREATE,  NOT_ALLOWED },
>   { DBModelAction.SELECT,  ALLOWED },
>   { DBModelAction.INSERT,  ALLOWED },
>   { DBModelAction.ALTER,   ALLOWED },
>   { DBModelAction.DROP,ALLOWED },
>   { DBModelAction.INDEX,   ALLOWED },
>   { DBModelAction.LOCK,ALLOWED },
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  1ab5be35dcb7991f723c0bb885ed2a15c6f5873a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  447deaf5831675257be1bbb05934a1ab4826fe9e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  78742fdacd72531eed4a7dde86871a57cf54493b 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestDescribeMetadataPrivileges.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68332/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68332: SENTRY-2352: User roles with ALTER on a table can not show or describe the table on which they have ALTER

[Sergio Pena via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

Added the following matrix of privileges on the DESCRIBE TABLE and SHOW TABLES:

DESCRIBE table allowed and not allowed privileges:

  { null,  NOT_ALLOWED }, // Means no privileges
  { DBModelAction.ALL, ALLOWED },
  { DBModelAction.CREATE,  NOT_ALLOWED },
  { DBModelAction.SELECT,  ALLOWED },
  { DBModelAction.INSERT,  ALLOWED },
  { DBModelAction.ALTER,   ALLOWED },
  { DBModelAction.DROP,NOT_ALLOWED },
  { DBModelAction.INDEX,   NOT_ALLOWED },
  { DBModelAction.LOCK,NOT_ALLOWED },
  
SHOW TABLES allowed and not allowed privileges:

  { null,  NOT_ALLOWED }, // Means no privileges
  { DBModelAction.ALL, ALLOWED },
  { DBModelAction.CREATE,  NOT_ALLOWED },
  { DBModelAction.SELECT,  ALLOWED },
  { DBModelAction.INSERT,  ALLOWED },
  { DBModelAction.ALTER,   ALLOWED },
  { DBModelAction.DROP,ALLOWED },
  { DBModelAction.INDEX,   ALLOWED },
  { DBModelAction.LOCK,ALLOWED },


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 1ab5be35dcb7991f723c0bb885ed2a15c6f5873a 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf5831675257be1bbb05934a1ab4826fe9e 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
 78742fdacd72531eed4a7dde86871a57cf54493b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestDescribeMetadataPrivileges.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
 PRE-CREATION 


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


Testing
---


Thanks,

Sergio Pena

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

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

(Updated Aug. 8, 2018, 10:36 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168b1970144dbf0b7a7378f2d25e70f1761d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
 61becceac881443b02182e6ab1012add4c046499 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
 6479a6055e8c7087f0e484080ec9d46a9c146212 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/Constants.java
 6e91f8b9ead009629d6bccd206122b2071e8fd64 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
 307f38eadb65bf12dc6225cfe43a5d590657d055 


Diff: https://reviews.apache.org/r/68268/diff/3/

Changes: https://reviews.apache.org/r/68268/diff/2-3/


Testing
---

I run the patch in a cluster and the audit logs is displaying the correct 
messages.

{"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
 privilege granted to USER: 
sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}


Thanks,

Sergio Pena

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

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

(Updated Aug. 8, 2018, 10:10 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Changes
---

The new output shows:

{"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_OWNER_PRIVILEGE","eventTime":"1533765994895","operationText":"OWNER
 privilege on  table 'db2.t1'
is granted to USER: 
sergio","allowed":"true","databaseName":"db2","tableName":"t1","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}

If a transferred happens, then the message will display 'OWNER PRIVILEGE on 
table db2.t1 is transffered to USER: sergio'


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


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
 ef63a34df1c90a08fc397a22454b6be176bae6cf 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
 e261fc6f66e29174c13940c70bdcf0caaa5290fa 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java
 ee4fce56ec18092dca49036263e7f2590ba9fb66 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 c1beaed0a6c76ae6cc60bde241b1aa2deae030a8 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeChanges.java
 abcf3ca5d813c8ae04413111d3decdbf587c693d 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeEntity.java
 85f81475192c58535700a0ca078bfbe25ac1fac4 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeEntityType.java
 ac44c1f91396f96d6dec52912f3fade089eeb845 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
 5691933fe85688f4f064c8c25e181c2857b1e6f3 
  sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift 
61582cd0eb9a8c096d84fa97e0d7639605f6265e 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java
 8bd9d439bcca7bdb6b94375a21b98388e2dcdb99 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
 c162ec19cbc40fc2a8dc6192f5af2db82e90336e 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 761c760f2f982e2ce580a1e0af0295d735d9fb5b 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
 f0ca787bb0c98c0ecbfebe4f8bba1a0fb499eec5 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 d47b9f73ee92de49bd785e9587ce47df5b2d3a80 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
 2a675041811ece7147fdd3c1bef767ac27113a2c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 6cb787bb7f61f0ee3e536d073757ba8758ae8fa6 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java
 d7bc748345638c49f341404b356d8a1944c2612f 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 b86136d6fe857aa1a87214550276f7b6b0800de9 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 845c1377d3756fe2df794d79a5fcb3d0e9f2eb31 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TListSentryPrivilegesRequest.java
 01e52300e2eba72aa4f7fa3e9ecab77aa0737d4b 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java
 fe891252eec81b28651106ea73a9480d42d5a0b7 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryObjectOwnerType.java
 6b540b88241abf4cb87246082b481f2b4c6fdca2 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
 1a8034b10f381395ca93c81cd432505d5fdcb5c8 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 07da2ba3caf9f5e5f1191ab7d3843ab4fd5dde33 
  
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 f238748ccc5c1f1b6157ec04a0b1d3211f997ccf 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyServiceClientDefaultImpl.java
 

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 7:25 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1577 (patched)
> > 
> >
> > should this be audit.onUpdateOwnerPrivilege()?
> 
> Sergio Pena wrote:
> Update implies a grant, so I'd rather use grant to avoid having two 
> methods that do the same.
> 
> Na Li wrote:
> update implies a grant and also removing. It is alter, not just grant. It 
> is better to be accurate, so we can differentiate different scenarios. In 
> this way, it is easier to debug when something goes wrong. To do this, you 
> need to add more functions for update message such as "OWNER privilege is 
> transferred to USER" OR "OWNER privilege is transferred to ROLE" base on the 
> new owner type, not "OWNER privilege granted to USER"

Aaa true. I changed it to audit.onTransferOwnerPrivilege()


- Sergio


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


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 9:12 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
> > Lines 169 (patched)
> > 
> >
> > This function calls createCmdForImplicitGrantOwnerPrivilege without 
> > checking anything specific to owner privilege. Should this function's name 
> > reflect the fact that it is specific for owner privilege? Otherwise, it is 
> > easy to introduce bug that some other developer could call this function 
> > for privilege other than owner privilege. Then audit log is wrong

I modify the flags to avoid confusions.


- Sergio


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


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Line 1619 (original), 1571 (patched)
> > 
> >
> > Why don't we have an audit log for revoking owner privileges?
> 
> Sergio Pena wrote:
> Because there is not information about what is being revoked. The revoke 
> happens by the revokeOwnerPrivileges() call.
> 
> I initially though on breaking the code to allow auditing the revoke, but 
> something special of the OWNER privilege is that only one role or user can 
> have it and it cannot be revoked. So, if you grant the OWNER to a user or 
> role, it immediatly means nobody else has that privilege, so in this case, it 
> does not make sense to log that as the OWNER privilege is assigned to one 
> user or role.
> 
> Arjun Mishra wrote:
> So are you saying we can never revoke owner privileges? If yes then I am 
> ok with the change. I would think the owner privilege would get revoked by 
> deleting the hive object. And I know Sentry gets notified of it

The owner and any other privilege is deleted by Sentry when an object is 
deleted on HMS. That's (for some reason) hasn't been audited. We might want to 
audit that too.
And yes, the OWNER privilege cannot be revoked, it can only be transferred to 
another user.


- Sergio


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


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1503 (patched)
> > 
> >
> > Can we maintain the same consistency with other audit methods?Maybe 
> > just pass in the request object?
> > 
> > Also, Can you explain why onGrantOwnerPrivileges needs the Status 
> > parameter to be passed in?

I cannot keep the consistency unless I create a thrift object for user 
privileges. Because the owner privilege is granted implicitly by Sentry, then 
we don't need the Thrift object, so I had to have this method with those 
parameters.

The Status is used by the createJsonLogEntity() method to append the status.


> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Line 1619 (original), 1571 (patched)
> > 
> >
> > Why don't we have an audit log for revoking owner privileges?

Because there is not information about what is being revoked. The revoke 
happens by the revokeOwnerPrivileges() call.

I initially though on breaking the code to allow auditing the revoke, but 
something special of the OWNER privilege is that only one role or user can have 
it and it cannot be revoked. So, if you grant the OWNER to a user or role, it 
immediatly means nobody else has that privilege, so in this case, it does not 
make sense to log that as the OWNER privilege is assigned to one user or role.


> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
> > Lines 215 (patched)
> > 
> >
> > I like this. Was curious since this should return the command that the 
> > user ran. 
> > Is there some way to display that?

I thought that too, but how to know what happend was executed? We know it could 
be a create database or table, or alter database or table; but that's a command 
from Hive. This audit log is for Sentry (no Hive). The only way I thought so is 
to write that message.


- Sergio


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


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

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

(Updated Aug. 8, 2018, 2:29 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168b1970144dbf0b7a7378f2d25e70f1761d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
 61becceac881443b02182e6ab1012add4c046499 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
 6479a6055e8c7087f0e484080ec9d46a9c146212 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
 307f38eadb65bf12dc6225cfe43a5d590657d055 


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


Testing (updated)
---

I run the patch in a cluster and the audit logs is displaying the correct 
messages.

{"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
 privilege granted to USER: 
sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}


Thanks,

Sergio Pena

Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


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


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168b1970144dbf0b7a7378f2d25e70f1761d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
 61becceac881443b02182e6ab1012add4c046499 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
 6479a6055e8c7087f0e484080ec9d46a9c146212 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
 307f38eadb65bf12dc6225cfe43a5d590657d055 


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


Testing
---

I run the patch in a cluster and the audit logs is displaying the correct 
messages.


Thanks,

Sergio Pena

Re: Review Request 68249: SENTRY-2255: alter table set owner command can be executed only by user with proper privilege

[Sergio Pena via Review Board]

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



Looks good.

- Sergio Pena


On Aug. 7, 2018, 2:14 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68249/
> ---
> 
> (Updated Aug. 7, 2018, 2:14 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2255
> https://issues.apache.org/jira/browse/sentry-2255
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> 1) support grant option for DDL option. This is a new feature. Many places 
> have to be changed at sentry server and client to make it happen
> 2) Add privilege map in HiveAuthzPrivilegesMap, which is commentted out as 
> hive version integrated with Sentry does not support "ALTER TABLE SET OWNER"
> 3) Add more testing cases and verify owner privilege, which is disabled
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  f1cbbb6 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  f164b30 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  9350af0 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  ab55609 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java
>  73fcda8 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/NoAuthorizationProvider.java
>  be0830d 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  621ce89 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  8a10214 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4588963 
> 
> 
> Diff: https://reviews.apache.org/r/68249/diff/1/
> 
> 
> Testing
> ---
> 
> existing test cases in TestOwnerPrivileges passed
> 
> 
> Thanks,
> 
> Na Li
> 
>