Re: Review Request 72882: SENTRY-2558: Issue in creating full snapshot when the storage descriptor for a table is null.

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Sept. 17, 2020, 3:40 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72882/
> ---
> 
> (Updated Sept. 17, 2020, 3:40 p.m.)
> 
> 
> Review request for sentry and Na Li.
> 
> 
> Bugs: SENTRY-2558
> https://issues.apache.org/jira/browse/SENTRY-2558
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When the storage descriptor for the table is null, sentry throws NullPointer 
> Exception and fails to create a snapshot.
> 
> 020-09-17 15:08:43,647 ERROR 
> org.apache.sentry.service.thrift.FullUpdateInitializer: Failed to execute 
> task java.lang.NullPointerException at 
> org.apache.sentry.service.thrift.FullUpdateInitializer$TableTask.doTask(FullUpdateInitializer.java:369)
>  at 
> org.apache.sentry.service.thrift.FullUpdateInitializer$BaseTask$RetryStrategy.exec(FullUpdateInitializer.java:222)
>  at 
> org.apache.sentry.service.thrift.FullUpdateInitializer$BaseTask.call(FullUpdateInitializer.java:258)
>  at 
> org.apache.sentry.service.thrift.FullUpdateInitializer$BaseTask.call(FullUpdateInitializer.java:190)
>  at java.util.concurrent.FutureTask.run(FutureTask.java:266) 
> at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
>  at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
>  at java.lang.Thread.run(Thread.java:748) 2020-09-17 15:08:43,650 
> INFO org.apache.sentry.service.thrift.FullUpdateInitializer: For db =
  test_db4 table = test_tb5 total number of partitions = 0 2020-09-17 
15:08:43,657 INFO hive.metastore: Closed a connection to metastore, current 
connections: 8 2020-09-17 15:08:43,657 ERROR 
org.apache.sentry.service.thrift.FullUpdateInitializer: Failed to execute task 
java.util.concurrent.RejectedExecutionException: Task 
java.util.concurrent.FutureTask@68c2c9a6 rejected from 
java.util.concurrent.ThreadPoolExecutor@1a289856[Shutting down, pool size = 7, 
active threads = 7, queued tasks = 0, completed tasks = 9] at 
java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:2063)
 at 
java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:830) 
at 
java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1379)   
  at 
java.util.concurrent.AbstractExecutorService.submit(AbstractExecutorService.java:134)
 at 
org.apache.sentry.service.thrift.FullUpdateInitializer$TableTask.doTask(FullUpdateInit
 ializer.java:366) at 
org.apache.sentry.service.thrift.FullUpdateInitializer$BaseTask$RetryStrategy.exec(FullUpdateInitializer.java:222)
 at 
org.apache.sentry.service.thrift.FullUpdateInitializer$BaseTask.call(FullUpdateInitializer.java:258)
 at 
org.apache.sentry.service.thrift.FullUpdateInitializer$BaseTask.call(FullUpdateInitializer.java:190)
 at java.util.concurrent.FutureTask.run(FutureTask.java:266) at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) 
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) 
at java.lang.Thread.run(Thread.java:748)
> Instead, it should ignore the object and move-on.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  668a4ca05 
> 
> 
> Diff: https://reviews.apache.org/r/72882/diff/1/
> 
> 
> Testing
> ---
> 
> I instrumented the failure by manually updating HMS database made sure that 
> logs look similar to one in the description. I verified ths issue is not 
> observed with the fix made.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 72704: SENTRY-2422: HMS synchronization is causing multiple entries of the same ID in SENTRY_HMS_NOTIFICATION_ID

[Na Li via Review Board]

> On July 24, 2020, 4 p.m., kalyan kumar kalvagadda wrote:
> > Lina,
> > 
> > Do you why are we ssting this issue? Notication should be processed by one 
> > sentry lender and there is only one thread the processes the notfications 
> > and updates the database.

Kalyan,

This is deployed on cloud. Each cluster has its own Sentry server instance, and 
all sentry servers point to the same Sentry DB. Therefore, the leader in each 
cluster syncs HMS notifications, and save into the same table at the same 
database. That makes the number of duplicated entries to increase very fast. 
That is why we need to avoid the duplicates.


- Na


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


On July 23, 2020, 8:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72704/
> ---
> 
> (Updated July 23, 2020, 8:55 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2422
> https://issues.apache.org/jira/browse/sentry-2422
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> before saving notification ID, check to make sure only save it if it is 
> larger than the values of existing entries. We only track and use 
> max(Notification ID). So no need to persist duplicated values.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  980c8ad 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  fd14963 
> 
> 
> Diff: https://reviews.apache.org/r/72704/diff/1/
> 
> 
> Testing
> ---
> 
> add new unit tests, and they pass
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 72706: SENTRY-2557: Queries are running too slow after when there are more than 4k roles

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On July 23, 2020, 11:38 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72706/
> ---
> 
> (Updated July 23, 2020, 11:38 p.m.)
> 
> 
> Review request for sentry and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When there are more than 4k roles Hive queries are very slow below because of 
> the time it spends on the compile phase because of the delay in fetching the 
> permissions.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  980c8ad6 
> 
> 
> Diff: https://reviews.apache.org/r/72706/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 72704: SENTRY-2422: HMS synchronization is causing multiple entries of the same ID in SENTRY_HMS_NOTIFICATION_ID

[Na Li via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

before saving notification ID, check to make sure only save it if it is larger 
than the values of existing entries. We only track and use max(Notification 
ID). So no need to persist duplicated values.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 980c8ad 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 fd14963 


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


Testing
---

add new unit tests, and they pass


Thanks,

Na Li

Re: Review Request 71942: SENTRY-2545: Rolling back Privilege Cache to SimplePrivilegeCache does not work

[Na Li via Review Board]

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

(Updated Dec. 29, 2019, 5:23 a.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

The change in SENTRY-2539 uses TreePrivilegeCache as default implementation of 
privilege cache to improve authorization performance. However, rolling back to 
SimplePrivilegeCache does not work due to how Sentry creates the privilege 
cache.

The solution is to create the privilege cache based on configured class name 
and handle different constructor inputs properly. 

More test cases are added to verify that rollback works.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 a4b664b 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 bc7a554 
  
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java
 4615bd4 
  
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 86e7d14 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestEndToEndWithSimpleCache.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEndWithSimpleCache.java
 PRE-CREATION 


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

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


Testing
---

Add new tests and they all passed.


Thanks,

Na Li

Review Request 71942: SENTRY-2545: Rolling back Privilege Cache to SimplePrivilegeCache does not work

[Na Li via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

The change in SENTRY-2539 uses TreePrivilegeCache as default implementation of 
privilege cache to improve authorization performance. However, rolling back to 
SimplePrivilegeCache does not work due to how Sentry creates the privilege 
cache.

The solution is to create the privilege cache based on configured class name 
and handle different constructor inputs properly. 

More test cases are added to verify that rollback works.


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 a4b664b 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 bc7a554 
  
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java
 4615bd4 
  
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 86e7d14 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestEndToEndWithSimpleCache.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEndWithSimpleCache.java
 PRE-CREATION 


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


Testing
---

Add new tests and they all passed.


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java
 3881692 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4c09e68 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 9045773 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 cb201bb 


Diff: https://reviews.apache.org/r/71915/diff/11/

Changes: https://reviews.apache.org/r/71915/diff/10-11/


Testing
---

Performance test in 

1) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
  
  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

2) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java
 3881692 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4c09e68 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 9045773 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 cb201bb 


Diff: https://reviews.apache.org/r/71915/diff/10/

Changes: https://reviews.apache.org/r/71915/diff/9-10/


Testing
---

Performance test in 

1) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
  
  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

2) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 170 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line170>
> >
> > Second argument here is basically is partIndex. 
> > You sending a -1 and using it by incrementing by one.
> >     Instead, I think you should just pass 0.
> 
> Na Li wrote:
> If I pass 0 to the function getChildResourceValue(), the index will be 0 
> + 1 = 1, and the result will be wrong.
> 
> To make it work, I implemented the function getTopLevelResourceValue() 
> that is even simpler, and does not need caller to pass in any index. The 
> equivalent index is 0, which is (- 1 + 1) = 0.

added function getResourceValue() that can accept index value of 0.


- Na


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


On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> ---
> 
> (Updated Dec. 20, 2019, 9:27 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
> 
> 
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now, the PolicyEngine Interface only returns the list of privileges in 
> the form of String. As a result, every authorization check has to convert the 
> privilege string to privilege object even though the cached privilege objects 
> are of the correct type already.
> 
> We should add a new function that returns privilege object directly to avoid 
> the overhead of conversion.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  5c7f84f 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  90fcfc3 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  de88705 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  2940a1e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  8e09490 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java
>  6a8b871 
>   
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java
>  0a0e2f0 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java
>  6782089 
>   
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java
>  94e9919 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  b6a1faa 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java
>  7bc94c9 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java
>  fa28716 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  5b261e3 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java
>  504b5ea 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java
>  6c2737a 
>   
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java
>  a819bb0 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
>  4bb6d32 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
>  ddb4ec5 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivileg

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

& 
> > privObj.getParts().get(0).getKey().equalsIgnoreCase("server")) {
> >   return privObj.getParts().get(0).getValue().endsWith("+");
> > }
> > return false;
> >   }

Thanks for the suggestion. I did that before adding function getParts() in 
Privilege. After adding this function, I don't need to convert the privilege to 
string and can use your suggestion.


> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java
> > Lines 111-151 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191035#file2191035line111>
> >
> > Can you avoid code duplication?

removed duplication


> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
> > Lines 137-150 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191036#file2191036line137>
> >
> > This method is incomplete. If you intent that this API not be used 
> > adding a comment here may not help.
> > 
> >   @Override
> >   public ImmutableSet getPrivilegeObjects(Set 
> > groups, Set users,
> > ActiveRoleSet roleSet, Authorizable... authorizableHierarchy) {
> >  throw Exception(); // Appropriate exception
> >   }
> >   
> >   This way if someone calls this method they would know that it is not 
> > supported.

I have added comment in the interface ProviderBackend. 

Its caller is ResourceAuthorizationProvider, which does not know what cache to 
be used. Throwing exception for each authorization check is too expensive. 
ResourceAuthorizationProvider's behavior is to call getPrivilegeObjects(). If 
return set is empty, it will call getPrivileges()


- Na


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


On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> ---
> 
> (Updated Dec. 20, 2019, 9:27 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
> 
> 
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now, the PolicyEngine Interface only returns the list of privileges in 
> the form of String. As a result, every authorization check has to convert the 
> privilege string to privilege object even though the cached privilege objects 
> are of the correct type already.
> 
> We should add a new function that returns privilege object directly to avoid 
> the overhead of conversion.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  5c7f84f 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  90fcfc3 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  de88705 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  2940a1e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  8e09490 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java
>  6a8b871 
>   
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java
>  0a0e2f0 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java
>  6782089 
>   
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java
>  94e9919 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  b6a1faa 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java
>  7bc94c9 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/val

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

ssue.)


> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java
> > Lines 58 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191023#file2191023line58>
> >
> > can we use privilegeFactory here to create privilege instead of 
> > directly CommonPrivilege instance?

fixed.


> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 49 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line49>
> >
> > The code assumes that when the cache is being created all the 
> > privileges which are being cached are available upfront. But the general 
> > pattern of a privilege cache is such that it should support addPrivilege() 
> > and removePrivilege() methods.

The scenarios that this cache is used are when the cache is being created all 
the privileges which are being cached are available upfront. This is not 
assumption. This is the reality.
In hive, sentry client gets all privileges of that user for each command, and 
creates cache to store it. So all authorization checks for this command are 
using this cache. 
https://github.com/apache/sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java#L852


- Na


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


On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> ---
> 
> (Updated Dec. 20, 2019, 9:27 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
> 
> 
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now, the PolicyEngine Interface only returns the list of privileges in 
> the form of String. As a result, every authorization check has to convert the 
> privilege string to privilege object even though the cached privilege objects 
> are of the correct type already.
> 
> We should add a new function that returns privilege object directly to avoid 
> the overhead of conversion.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  5c7f84f 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  90fcfc3 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  de88705 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  2940a1e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  8e09490 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java
>  6a8b871 
>   
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java
>  0a0e2f0 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java
>  6782089 
>   
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java
>  94e9919 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  b6a1faa 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java
>  7bc94c9 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java
>  fa28716 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  5b261e3 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java
>  504b5ea 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java
>  6c2737a 
>   
> sentry-policy/sentry-policy-engi

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java
 3881692 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4c09e68 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 cc0465a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 cb201bb 


Diff: https://reviews.apache.org/r/71915/diff/9/

Changes: https://reviews.apache.org/r/71915/diff/8-9/


Testing
---

Performance test in 

1) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
  
  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

2) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java
 3881692 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4c09e68 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 cc0465a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 cb201bb 


Diff: https://reviews.apache.org/r/71915/diff/8/

Changes: https://reviews.apache.org/r/71915/diff/7-8/


Testing
---

Performance test in 

1) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
  
  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

2) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java
 3881692 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4c09e68 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 cc0465a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 cb201bb 


Diff: https://reviews.apache.org/r/71915/diff/7/

Changes: https://reviews.apache.org/r/71915/diff/6-7/


Testing
---

Performance test in 

1) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
  
  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

2) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
> > Line 852 (original), 852 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189390#file2189390line852>
> >
> > Can we make this change configurable? Right now, it seems that there is 
> > no way a user can revert to old behavior in case of any issues with the 
> > TreePrivilegeCache.

done


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Line 503 (original), 503 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189391#file2189391line503>
> >
> > Make this configurable?

done


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
> > Lines 33 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189404#file2189404line33>
> >
> > It looks like adding any new methods to this interface will break the 
> > clients who implement this? Is this not a public API? If yes, can we make 
> > changes to avoid backwards incompatibility?

move the new functions to a child interface of PrivilegeCache


- Na


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


On Dec. 18, 2019, 4:52 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> ---
> 
> (Updated Dec. 18, 2019, 4:52 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
> 
> 
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now, the PolicyEngine Interface only returns the list of privileges in 
> the form of String. As a result, every authorization check has to convert the 
> privilege string to privilege object even though the cached privilege objects 
> are of the correct type already.
> 
> We should add a new function that returns privilege object directly to avoid 
> the overhead of conversion.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  5c7f84f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  de88705 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  2940a1e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  8e09490 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java
>  6a8b871 
>   
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java
>  0a0e2f0 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java
>  6782089 
>   
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java
>  94e9919 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  b6a1faa 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java
>  7bc94c9 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java
>  fa28716 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  5b261e3 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java
>  504b5ea 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java
>  6c2737a 
>   
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java
>  a819bb0 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/Privi

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 163 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line163>
> >
> > please add comments int this method with examples for clarity. Also not 
> > very clear on why the index is offset by 1.

I replaced the check with the function hasChild(). The index is 0 based. So if 
the array size is 2, and the index value is 1, then the index already points to 
the end of the array. That is why there is offset by 1.


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 183 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line183>
> >
> > I think the usage of the word "key" in method/variable names for both 
> > authorizable and Privilege is very confusing. Can you please rename the 
> > variables and method names better?

I changed the function names to getChildResourceValue() and 
isResourceValueWildcard(). Hopefully it is easier to read the code now.


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 204 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line204>
> >
> > may be rename this to getChildAuthorizableName?

getChildResourceValue()


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line222>
> >
> > The method name suggests its returning the key but the implememtation 
> > returns the value. Can you please add a java doc on these methods to 
> > improve readability?

change the function name to getChildResourceValue(), which is used as key for 
the hashmap.


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 229 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line229>
> >
> > Its unclear why we are using partIndex+1. Can you add some comments to 
> > explain the same?

replace it with hasChild()


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java
> > Line 58 (original), 66 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189410#file2189410line66>
> >
> > Why was this change necessary?

the type of a column authorizable is "column". It is a typo to put the type as 
"col". The error was found after I added new test cases below

assertEquals(5, cache.listPrivileges(null, null, null, new 
Server("server1"), new Database("db1"), new Table("t1"), new 
Column("*")).size());
assertEquals(6, cache.listPrivileges(null, null, null, new 
Server("server1"), new Database("db1"), new Table("*"), new 
Column("*")).size());
assertEquals(2, cache.listPrivileges(null, null, null, new 
Server("server1"), new Database("db2"), new Table("t1"), new 
Column("*")).size());

1) The authorizable string is "server=server1->db=db1->table=t1->column=*" for 
`new Server("server1"), new Database("db1"), new Table("t1"), new Column("*")`
2) With the typo, 
  2.1) the privilege string is 
"server=server1->db=db1->table=t1->col=c1->action=select" for the privilege
   CommonPrivilege colSelect = create(new KeyValue("Server", "server1"),
new KeyValue("db", "db1"), new KeyValue("table", "t1"), new 
KeyValue("col", "c1"), new KeyValue("action", "SELECT"));

  2.2) This privilege does not match the input authorizable string "col" vs 
"column"

3) After fixing the typo, 
  3.1) the privilege string is 
"server=server1->db=db1->table=t1->column=c1->action=select" for the privilege  
  
   CommonPrivilege colSelect = create(new KeyValue("Server", "server1"),
ne

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
> > Lines 269 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189392#file2189392line269>
> >
> > not sure I understand this. Can you clarify why is this related to this 
> > patch?

In this function SentryMetaStoreFilterHook.filterTab(), you can see 
authzBinding.close() is called for each filtering operation. If its cache is 
cleared at close() and the same HiveMetaStoreClient is used for more than one 
filtering operation, the next filtering operation will fail.

In TreePrivilegeCache.close(), the cachedPrivileges and cachedPrivilegeMap are 
cleared. So there is no cached privileges, and the test 
TestMetastoreEndToEnd.testListTables failed when the same HiveMetaStoreClient 
is used to filter more than once.

On the other hand, SimplePrivilegeCache.close(), the cachedPrivileges is 
cleared, but cachedAuthzPrivileges is not cleared. That is why the test 
TestMetastoreEndToEnd.testListTables did not fail for it. 

I believe the correct behavior for cache is to clear its cache at close(). 
Therefore, we need to make sure SentryMetaStoreFilterHook.filterTab() will 
clear its hiveAuthzBinding, and get new privileges for its cache in next 
operation.


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
> > Lines 26-27 (original), 26-27 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189397#file2189397line26>
> >
> > Do you think we should intern these? Seems like there would be a lot of 
> > duplicates otherwise.

done


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189407#file2189407line42>
> >
> > Do we need to cache strings as well since we are generating 
> > CachedPrivilegeMap?

it is there to suport old API such as "listPrivileges(Set groups, 
ActiveRoleSet roleSet)"


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 48 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line48>
> >
> > change inPrivilege to final?

done


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line113>
> >
> > targetSet seems to be a local variable on the stack. Why do we need to 
> > create a copyOf it?

You are right. There is no need. removed the copy.


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 116 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line116>
> >
> > This method has exact sane signature of with listPrivilegeObjects. What 
> > is the difference between the two?

added comment to show the different functionalities.


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 126 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line126>
> >
> > Do we have to distinguish the cases for different wildcards?

Yes. When the authorizable part is wildcard, it should get privileges of all 
children. For example, in "show tables" command, the cache should return all 
column level privileges. The column = "*"

requiredInputPrivileges [SELECT, INSERT, ALTER, CREATE, DROP, INDEX, LOCK]
inputHierarchyList = [[Server [name=server1], Database [name=db_1], Table 
[name=tab1], Column [name=*]]]

If the authorizable part is not wildcard, it should only get privileges of that 
specific child.


- Na


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


On Dec. 18, 2019, 4:52 p.m., Na Li wrote:
> 
> ---
> This is an automaticall

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

/diff/6/


Testing (updated)
---

Performance test in 

1) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
  
  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

2) 
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

/71915/diff/6/

Changes: https://reviews.apache.org/r/71915/diff/5-6/


Testing
---

Performance test in 

Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]

  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

], table[10]
Authorizable: db[12000], table[1]

  TreePrivilegeCache - total time on list string: 448 ms 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 365 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 717 ms   
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 794 ms  
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)

Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]

  TreePrivilegeCache - total time on list string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 string: 1302 ms
(TestTreePrivilegeCache.testListPrivilegesPerf)
  TreePrivilegeCache - total time on list obj: 604 ms
(TestTreePrivilegeCache.testListPrivilegeObjectsPerf)

  SimplePrivilegeCache - total time on list string: 4436 ms  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

  
(TestSimplePrivilegeCache.testListPrivilegesPerf)
  SimplePrivilegeCache - total time on list obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

 obj: 3732 ms 
(TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)


Thanks,

Na Li

Re: Review Request 71901: SENTRY-2540: Only use SELECT action for filter SHOW DATABASES and SHOW TABLES command based on configuration

[Na Li via Review Board]

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

(Updated Dec. 13, 2019, 4:38 p.m.)


Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.


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


Repository: sentry


Description
---

When there are thousands of databases, SHOW DATABASES may take a really long 
time because SENTRY checks if user has any of the following privileges on that 
database for filtering out the database

DBModelAction.SELECT, DBModelAction.INSERT, DBModelAction.ALTER,
DBModelAction.CREATE, DBModelAction.DROP, DBModelAction.INDEX,
DBModelAction.LOCK

To speedup the authorization checking for this case, Sentry can check only the 
select privilege for SHOW DATABASES and SHOW TABLES based on configuration.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 5c43329 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 e64d1a5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 cc0465a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
 6a88d0b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivilegesOnSelectOnly.java
 PRE-CREATION 


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

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


Testing
---

manually set the configuration to be true, and see only select action is used 
for authorization check


Thanks,

Na Li

Re: Review Request 71901: SENTRY-2540: Only use SELECT action for filter SHOW DATABASES and SHOW TABLES command based on configuration

[Na Li via Review Board]

> On Dec. 12, 2019, 6:02 p.m., kalyan kumar kalvagadda wrote:
> > Code change looks good. Please add unit tests to cover the same.

tests are added for the new behavior both default and enabled


- Na


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


On Dec. 13, 2019, 12:23 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71901/
> ---
> 
> (Updated Dec. 13, 2019, 12:23 a.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
> 
> 
> Bugs: sentry-2540
> https://issues.apache.org/jira/browse/sentry-2540
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When there are thousands of databases, SHOW DATABASES may take a really long 
> time because SENTRY checks if user has any of the following privileges on 
> that database for filtering out the database
> 
> DBModelAction.SELECT, DBModelAction.INSERT, DBModelAction.ALTER,
> DBModelAction.CREATE, DBModelAction.DROP, DBModelAction.INDEX,
> DBModelAction.LOCK
> 
> To speedup the authorization checking for this case, Sentry can check only 
> the select privilege for SHOW DATABASES and SHOW TABLES based on 
> configuration.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  5c43329 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
>  e64d1a5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  cc0465a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
>  6a88d0b 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivilegesOnSelectOnly.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71901/diff/2/
> 
> 
> Testing
> ---
> 
> manually set the configuration to be true, and see only select action is used 
> for authorization check
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 71901: SENTRY-2540: Only use SELECT action for filter SHOW DATABASES and SHOW TABLES command based on configuration

[Na Li via Review Board]

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

(Updated Dec. 13, 2019, 12:23 a.m.)


Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.


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


Repository: sentry


Description
---

When there are thousands of databases, SHOW DATABASES may take a really long 
time because SENTRY checks if user has any of the following privileges on that 
database for filtering out the database

DBModelAction.SELECT, DBModelAction.INSERT, DBModelAction.ALTER,
DBModelAction.CREATE, DBModelAction.DROP, DBModelAction.INDEX,
DBModelAction.LOCK

To speedup the authorization checking for this case, Sentry can check only the 
select privilege for SHOW DATABASES and SHOW TABLES based on configuration.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 5c43329 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 e64d1a5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 cc0465a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
 6a88d0b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivilegesOnSelectOnly.java
 PRE-CREATION 


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

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


Testing
---

manually set the configuration to be true, and see only select action is used 
for authorization check


Thanks,

Na Li

Review Request 71901: SENTRY-2540: Only use SELECT action for filter SHOW DATABASES and SHOW TABLES command based on configuration

[Na Li via Review Board]

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

Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.


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


Repository: sentry


Description
---

When there are thousands of databases, SHOW DATABASES may take a really long 
time because SENTRY checks if user has any of the following privileges on that 
database for filtering out the database

DBModelAction.SELECT, DBModelAction.INSERT, DBModelAction.ALTER,
DBModelAction.CREATE, DBModelAction.DROP, DBModelAction.INDEX,
DBModelAction.LOCK

To speedup the authorization checking for this case, Sentry can check only the 
select privilege for SHOW DATABASES and SHOW TABLES based on configuration.


Diffs
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 5c43329 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 e64d1a5 


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


Testing
---

manually set the configuration to be true, and see only select action is used 
for authorization check


Thanks,

Na Li

Re: Review Request 71838: SENTRY-2538: consecutiveUpdateFailuresCount is not reset

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 27, 2019, 4:28 p.m., László Terjéki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71838/
> ---
> 
> (Updated Nov. 27, 2019, 4:28 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2538: consecutiveUpdateFailuresCount is not reset
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  a57e2ee9 
> 
> 
> Diff: https://reviews.apache.org/r/71838/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> László Terjéki
> 
>

Re: Review Request 70846: SENTRY-2240: User can DROP function under a database that he/she has no access

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On June 14, 2019, 2:58 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70846/
> ---
> 
> (Updated June 14, 2019, 2:58 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Na Li.
> 
> 
> Bugs: SENTRY-2240
> https://issues.apache.org/jira/browse/SENTRY-2240
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> User can DROP UDF function under a database that he/she has no access to.
> 
> I created it as separate JIRA from SENTRY-781 due to changes are quite 
> different.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  e87d0f664fd6cf93b3b86a61b57f148827e5692f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  ed278c8d68c415198f40bed62cfa757fa5a9 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  1aaa9b3fcade6ebcefcea269b3bd919fb47a44f6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
>  bd0f978e86733b37cf3343c9841304fd61f9dcab 
> 
> 
> Diff: https://reviews.apache.org/r/70846/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 70846: SENTRY-2240: User can DROP function under a database that he/she has no access

[Na Li via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Line 230 (original), 226 (patched)
<https://reviews.apache.org/r/70846/#comment302841>

change comment to be consistent



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 228 (patched)
<https://reviews.apache.org/r/70846/#comment302842>

do you want to change for create function as well to get true currDB?

  case HiveParser.TOK_CREATEFUNCTION:
String udfClassName = 
BaseSemanticAnalyzer.unescapeSQLString(ast.getChild(1).getText());
try {
  CodeSource udfSrc =
  Class.forName(udfClassName, true, 
Utilities.getSessionSpecifiedClassLoader())
  .getProtectionDomain().getCodeSource();
  if (udfSrc == null) {
throw new SemanticException("Could not resolve the jar for UDF 
class " + udfClassName);
  }
  String udfJar = udfSrc.getLocation().getPath();
  if (udfJar == null || udfJar.isEmpty()) {
throw new SemanticException("Could not find the jar for UDF 
class " + udfClassName +
"to validate privileges");
  }
  udfURIs.add(parseURI(udfSrc.getLocation().toString(), true));
} catch (ClassNotFoundException e) {
  List functionJars = getFunctionJars(ast);
  if (functionJars.isEmpty()) {
throw new SemanticException("Error retrieving udf class:" + 
e.getMessage(), e);
  } else {
// Add the jars from the command "Create function using jar" to 
the access list
// Defer to hive to check if the class is in the jars
for(String jar : functionJars) {
  udfURIs.add(parseURI(jar, false));
}
  }
}

// create/drop function is allowed with any database
currDB = Database.ALL;



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
Line 120 (original), 123 (patched)
<https://reviews.apache.org/r/70846/#comment302843>

can you add test case for creating function fail due to not having 
privilege?


- Na Li


On June 14, 2019, 2:58 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70846/
> ---
> 
> (Updated June 14, 2019, 2:58 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Na Li.
> 
> 
> Bugs: SENTRY-2240
> https://issues.apache.org/jira/browse/SENTRY-2240
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> User can DROP UDF function under a database that he/she has no access to.
> 
> I created it as separate JIRA from SENTRY-781 due to changes are quite 
> different.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  e87d0f664fd6cf93b3b86a61b57f148827e5692f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  ed278c8d68c415198f40bed62cfa757fa5a9 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  1aaa9b3fcade6ebcefcea269b3bd919fb47a44f6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
>  bd0f978e86733b37cf3343c9841304fd61f9dcab 
> 
> 
> Diff: https://reviews.apache.org/r/70846/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Na Li via Review Board]

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

(Updated March 4, 2019, 9:49 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

When granting all privilege on a table from impala-shell, this privilege cannot 
be revoked from hiove beeline.

When granting all privilege on a table from hive beeline, this privilege cannot 
be revoked from impala-shell.

The reason is that impala uses "ALL" to represent all privilege, and hive uses 
"*". So getting privilege to drop it does not properly.

The solution is to find all equivalent privileges from input, and drop them.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 240120c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1d97ff6 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 38b4c87 


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

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


Testing
---

add new tests and all old tests in TestSentryStore pass


Thanks,

Na Li

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Na Li via Review Board]

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

(Updated March 4, 2019, 8:46 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

When granting all privilege on a table from impala-shell, this privilege cannot 
be revoked from hiove beeline.

When granting all privilege on a table from hive beeline, this privilege cannot 
be revoked from impala-shell.

The reason is that impala uses "ALL" to represent all privilege, and hive uses 
"*". So getting privilege to drop it does not properly.

The solution is to find all equivalent privileges from input, and drop them.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 240120c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1d97ff6 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 38b4c87 


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

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


Testing
---

add new tests and all old tests in TestSentryStore pass


Thanks,

Na Li

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Na Li via Review Board]

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

(Updated March 1, 2019, 4:01 a.m.)


Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

When granting all privilege on a table from impala-shell, this privilege cannot 
be revoked from hiove beeline.

When granting all privilege on a table from hive beeline, this privilege cannot 
be revoked from impala-shell.

The reason is that impala uses "ALL" to represent all privilege, and hive uses 
"*". So getting privilege to drop it does not properly.

The solution is to find all equivalent privileges from input, and drop them.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 240120c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1d97ff6 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 38b4c87 


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

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


Testing
---

add new tests and all old tests in TestSentryStore pass


Thanks,

Na Li

Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Na Li via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

When granting all privilege on a table from impala-shell, this privilege cannot 
be revoked from hiove beeline.

When granting all privilege on a table from hive beeline, this privilege cannot 
be revoked from impala-shell.

The reason is that impala uses "ALL" to represent all privilege, and hive uses 
"*". So getting privilege to drop it does not properly.

The solution is to find all equivalent privileges from input, and drop them.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 240120c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1d97ff6 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 38b4c87 


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


Testing
---

add new tests and all old tests in TestSentryStore pass


Thanks,

Na Li

Re: Review Request 70043: SENTRY-2502: Sentry NN plug-in stops fetching updates from sentry server

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Feb. 22, 2019, 6:40 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70043/
> ---
> 
> (Updated Feb. 22, 2019, 6:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2502
> https://issues.apache.org/jira/browse/SENTRY-2502
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry plug-in in name node is stopping to fetch updates from sentry server 
> when below sequence of events occurs.
> 
> Create a table
> Add a single partition to it.
> Rename the table created above.
> Drop the partition added above
> Add the partition again.
> Rename the table
> 
> There are a couple of issues observed
> 
> When the table is renamed the Entry object for the table that is renamed is 
> wrongly update. Type of Entry is changed to "DIR".
> When paths have removed the entries with empty paths are not removed from the 
> collection of Entries for an object.
> These two issues can manifest in multiple problems. Issue reported in this 
> Jira is one such problem.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  1dfa1d9aafbd482903e4bb935509a76768e02b58 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
>  8a7c8a2901af18b375f8637572d9b3ddee0ded6c 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9fe22ef8cc1533b28462e2b8749e3ba0e7b56582 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  574bc4b0432824abc81fe3c0dd30d1fe01f012e5 
> 
> 
> Diff: https://reviews.apache.org/r/70043/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test case to verify that issues that does not occur after the fix.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 70043: SENTRY-2502: Sentry NN plug-in stops fetching updates from sentry server

[Na Li via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 507 (patched)
<https://reviews.apache.org/r/70043/#comment298962>

should this be removed if it has no authz obj?

Why change its type to DIR?


- Na Li


On Feb. 22, 2019, 6:40 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70043/
> ---
> 
> (Updated Feb. 22, 2019, 6:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2502
> https://issues.apache.org/jira/browse/SENTRY-2502
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry plug-in in name node is stopping to fetch updates from sentry server 
> when below sequence of events occurs.
> 
> Create a table
> Add a single partition to it.
> Rename the table created above.
> Drop the partition added above
> Add the partition again.
> Rename the table
> 
> There are a couple of issues observed
> 
> When the table is renamed the Entry object for the table that is renamed is 
> wrongly update. Type of Entry is changed to "DIR".
> When paths have removed the entries with empty paths are not removed from the 
> collection of Entries for an object.
> These two issues can manifest in multiple problems. Issue reported in this 
> Jira is one such problem.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  1dfa1d9aafbd482903e4bb935509a76768e02b58 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
>  8a7c8a2901af18b375f8637572d9b3ddee0ded6c 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9fe22ef8cc1533b28462e2b8749e3ba0e7b56582 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  574bc4b0432824abc81fe3c0dd30d1fe01f012e5 
> 
> 
> Diff: https://reviews.apache.org/r/70043/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test case to verify that issues that does not occur after the fix.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 70043: SENTRY-2502: Sentry NN plug-in stops fetching updates from sentry server

[Na Li via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 689 (original), 694 (patched)
<https://reviews.apache.org/r/70043/#comment298961>

Kalyan, do you mean "If we use the updated Entry to find/delete it from the 
set it will NOT be found/delted." instead of "If we use the updated Entry to 
find/delete it from the set it will be found/delted."?

Your change has performance penalty. Do you have an idea how big the 
performance degradation could it be? and will we be able to accept that?


- Na Li


On Feb. 22, 2019, 6:40 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70043/
> ---
> 
> (Updated Feb. 22, 2019, 6:40 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2502
> https://issues.apache.org/jira/browse/SENTRY-2502
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry plug-in in name node is stopping to fetch updates from sentry server 
> when below sequence of events occurs.
> 
> Create a table
> Add a single partition to it.
> Rename the table created above.
> Drop the partition added above
> Add the partition again.
> Rename the table
> 
> There are a couple of issues observed
> 
> When the table is renamed the Entry object for the table that is renamed is 
> wrongly update. Type of Entry is changed to "DIR".
> When paths have removed the entries with empty paths are not removed from the 
> collection of Entries for an object.
> These two issues can manifest in multiple problems. Issue reported in this 
> Jira is one such problem.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  1dfa1d9aafbd482903e4bb935509a76768e02b58 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
>  8a7c8a2901af18b375f8637572d9b3ddee0ded6c 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9fe22ef8cc1533b28462e2b8749e3ba0e7b56582 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  574bc4b0432824abc81fe3c0dd30d1fe01f012e5 
> 
> 
> Diff: https://reviews.apache.org/r/70043/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test case to verify that issues that does not occur after the fix.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Feb. 21, 2019, 8:17 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 21, 2019, 8:17 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/2/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 70013: SENTRY-2501: Add cache for HMS server filtering hook

[Na Li via Review Board]

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

(Updated Feb. 20, 2019, 9:59 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

The filter in SentryMetaStoreFilterHook does not cache sentry privileges. 
Therefore, for each item in the list, sentry client has to get privileges from 
sentry server.

To improve performance, we need to add cache in SentryMetaStoreFilterHook


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 cdb6de4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
 312c5db 


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

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


Testing
---

existing HMS tests succeeded


Thanks,

Na Li

Re: Review Request 70008: SENTRY-2500: CREATE on server does not provide HMS server side read authorization for get_all_tables(database_name)

[Na Li via Review Board]

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

(Updated Feb. 20, 2019, 9:39 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

CREATE privilege is added for listing tables. So create on server can get list 
of table names


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 178780e 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 3ca89be 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 1f7148b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 d63957a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/StaticUserGroup.java
 8306e95 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
 88e697b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f1600c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 6327f16 


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

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


Testing
---

existing tests for metastore, and add two new tests for reading database and 
tables.


Thanks,

Na Li

Re: Review Request 70013: SENTRY-2501: Add cache for HMS server filtering hook

[Na Li via Review Board]

> On Feb. 20, 2019, 3:36 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Lines 479-506 (patched)
> > <https://reviews.apache.org/r/70013/diff/1/?file=2125866#file2125866line479>
> >
> > Functionally I'm good with the change but it is a code duplication as 
> > we will have same implementation in MetastoreAuthzBindingBase and 
> > HiveAuthzBindingHookBase.
> > 
> > Could you move this caching to an abstract class and have these classes 
> > reuse it?

what is the abstract class would you suggest?

HiveAuthzBindingHookBase extends AbstractSemanticAnalyzerHook, and 
MetastoreAuthzBindingBase extends MetaStorePreEventListener. They don't have 
comment parent.

If I force them to derive from the same class, which extends 
AbstractSemanticAnalyzerHook and MetaStorePreEventListener, the inherantance 
structure would be very twisted just to share a common implementatioon of a 
function. I don't feel comfortable for that.


- Na


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


On Feb. 19, 2019, 9:59 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70013/
> ---
> 
> (Updated Feb. 19, 2019, 9:59 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2501
> https://issues.apache.org/jira/browse/sentry-2501
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The filter in SentryMetaStoreFilterHook does not cache sentry privileges. 
> Therefore, for each item in the list, sentry client has to get privileges 
> from sentry server.
> 
> To improve performance, we need to add cache in SentryMetaStoreFilterHook
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  cdb6de4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  312c5db 
> 
> 
> Diff: https://reviews.apache.org/r/70013/diff/1/
> 
> 
> Testing
> ---
> 
> existing HMS tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 70013: SENTRY-2501: Add cache for HMS server filtering hook

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

The filter in SentryMetaStoreFilterHook does not cache sentry privileges. 
Therefore, for each item in the list, sentry client has to get privileges from 
sentry server.

To improve performance, we need to add cache in SentryMetaStoreFilterHook


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 cdb6de4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
 312c5db 


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


Testing
---

existing HMS tests succeeded


Thanks,

Na Li

Re: Review Request 70008: SENTRY-2500: CREATE on server does not provide HMS server side read authorization for get_all_tables(database_name)

[Na Li via Review Board]

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

(Updated Feb. 19, 2019, 9:56 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

CREATE privilege is added for listing tables. So create on server can get list 
of table names


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 178780e 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 3ca89be 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 1f7148b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 d63957a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/StaticUserGroup.java
 8306e95 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
 88e697b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f1600c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 6327f16 


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

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


Testing
---

existing tests for metastore, and add two new tests for reading database and 
tables.


Thanks,

Na Li

Re: Review Request 70008: SENTRY-2500: CREATE on server does not provide HMS server side read authorization for get_all_tables(database_name)

[Na Li via Review Board]

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

(Updated Feb. 19, 2019, 7:12 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

CREATE privilege is added for listing tables. So create on server can get list 
of table names


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 178780e 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 3ca89be 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 1f7148b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 d63957a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/StaticUserGroup.java
 8306e95 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f1600c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 6327f16 


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

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


Testing
---

existing tests for metastore, and add two new tests for reading database and 
tables.


Thanks,

Na Li

Re: Review Request 70008: SENTRY-2500: CREATE on server does not provide HMS server side read authorization for get_all_tables(database_name)

[Na Li via Review Board]

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

(Updated Feb. 19, 2019, 5:01 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

CREATE privilege is added for listing tables. So create on server can get list 
of table names


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 178780e 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 3ca89be 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 1f7148b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 d63957a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/StaticUserGroup.java
 8306e95 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f1600c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 6327f16 


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

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


Testing
---

existing tests for metastore, and add two new tests for reading database and 
tables.


Thanks,

Na Li

Review Request 70008: SENTRY-2500: CREATE on server does not provide HMS server side read authorization for get_all_tables(database_name)

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

CREATE privilege is added for listing tables. So create on server can get list 
of table names


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
 178780e 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
 3ca89be 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
 1f7148b 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 d63957a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/StaticUserGroup.java
 8306e95 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 6327f16 


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


Testing
---

existing tests for metastore, and add two new tests for reading database and 
tables.


Thanks,

Na Li

Re: Review Request 69924: SENTRY-2440: Add a new thrift API for checking if a user is in admin group

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Feb. 7, 2019, 11:47 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69924/
> ---
> 
> (Updated Feb. 7, 2019, 11:47 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2440: Add a new thrift API for checking if a user is in admin group to 
> check if a given user is in the Sentry admin group or not.
> 
> This is useful for Sentry client to recognize failure earlier than actually 
> making a call to privileged API such as 'create_role', 'drop_role'.
> 
> This API shouldn't leak any sensitive information because connection to teh 
> Sentry server is guarded by 'sentry.service.allow.connect' config, that only 
> the trusted service users can connect to the Sentry service.
> 
> 
> 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/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TIsSentryAdminRequest.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TIsSentryAdminResponse.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
>  5fc299b2485e0af6df333e4a288299f39e18b3b7 
>   
> 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
>  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
>  30875299ebf81e74a78b396d4aeaf27890083370 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceIntegration.java
>  bfafa7d99735bec07bd81ebe665f4e84e65bd3b7 
> 
> 
> Diff: https://reviews.apache.org/r/69924/diff/1/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 69536: SENTRY-2471: Table rename should sync Sentry privilege even without location information

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Feb. 7, 2019, 1:04 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69536/
> ---
> 
> (Updated Feb. 7, 2019, 1:04 a.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2471: Table rename should sync Sentry privilege even without location 
> information
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  7b7d0e1eb7ea1f17dea622e51ccc557e0b76fbff 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestNotificationProcessor.java
>  f227bb45835b98e6fef14399a239d7f9975d56d1 
> 
> 
> Diff: https://reviews.apache.org/r/69536/diff/3/
> 
> 
> Testing
> ---
> 
> added new unit tests
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Jan. 29, 2019, 9:09 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69076/
> ---
> 
> (Updated Jan. 29, 2019, 9:09 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2301
> https://issues.apache.org/jira/browse/SENTRY-2301
> 
> 
> 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-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  2d21411e2 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  534bb51c1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  4ff3dc91a 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4baeb6725 
> 
> 
> Diff: https://reviews.apache.org/r/69076/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69573: SENTRY-2477: When requesting for deltas check if nn seq num is 1 more than latest sequence num

[Na Li via Review Board]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 136 (original), 136 (patched)
<https://reviews.apache.org/r/69573/#comment298420>

Can you change the comment to include the situation that "seqNum > 
curSeqNum +1", and under what situation, it may happen? 

You don't need to wait for my addition approval before commiting the change


- Na Li


On Dec. 17, 2018, 5:07 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69573/
> ---
> 
> (Updated Dec. 17, 2018, 5:07 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2477
> https://issues.apache.org/jira/browse/SENTRY-2477
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If NN seq number and latest sentry sequence number is larger than 1 we need 
> to request a full update
> 
> 
> 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/69573/diff/1/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestDBUpdateForwarder
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69573: SENTRY-2477: When requesting for deltas check if nn seq num is 1 more than latest sequence num

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 17, 2018, 5:07 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69573/
> ---
> 
> (Updated Dec. 17, 2018, 5:07 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2477
> https://issues.apache.org/jira/browse/SENTRY-2477
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If NN seq number and latest sentry sequence number is larger than 1 we need 
> to request a full update
> 
> 
> 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/69573/diff/1/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestDBUpdateForwarder
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

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

[Na Li via Review Board]

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

(Updated Jan. 30, 2019, 5:02 a.m.)


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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f14cbb6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


Diff: https://reviews.apache.org/r/69702/diff/8/

Changes: https://reviews.apache.org/r/69702/diff/7-8/


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

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

[Na Li via Review Board]

> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/69702/diff/7/?file=2123178#file2123178line113>
> >
> > Don't you think, 
> > senry.metastore.read.authorization.enabled is better?

AGREE.


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Line 158 (original), 163 (patched)
> > <https://reviews.apache.org/r/69702/diff/7/?file=2123179#file2123179line163>
> >
> > Why is change needed?

user name should be case sensitive


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Lines 212 (patched)
> > <https://reviews.apache.org/r/69702/diff/7/?file=2123179#file2123179line212>
> >
> > How about authorizeDatabaseRead?

the event type is read_database. Using authorizeReadDatabase seems clearer for 
me


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Lines 217 (patched)
> > <https://reviews.apache.org/r/69702/diff/7/?file=2123179#file2123179line217>
> >
> > How about authorizeTableRead?

the event type is read_table, that is why authorizeReadTable is used


- Na


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


On Jan. 29, 2019, 4:15 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 29, 2019, 4:15 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
> 
> 
> 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.
> 
> This is based on changed made by Sergio at 
> https://reviews.apache.org/r/69620/, and add code to fix unstable e2e tests
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  9efd612 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   sentry-tests/sentry-tests-hive/pom.xml 74777bb 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
>  f14cbb6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
>  3c28fd0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304f 
> 
> 
> Diff: https://reviews.apache.org/r/69702/diff/7/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

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

[Na Li via Review Board]

> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Line 158 (original), 162 (patched)
> > <https://reviews.apache.org/r/69702/diff/4/?file=2122038#file2122038line162>
> >
> > Why is thie removed?

User name should be case sensitive. See details in 
https://issues.apache.org/jira/browse/SENTRY-2432


- Na


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


On Jan. 29, 2019, 4:15 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 29, 2019, 4:15 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
> 
> 
> 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.
> 
> This is based on changed made by Sergio at 
> https://reviews.apache.org/r/69620/, and add code to fix unstable e2e tests
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  9efd612 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   sentry-tests/sentry-tests-hive/pom.xml 74777bb 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
>  f14cbb6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
>  3c28fd0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304f 
> 
> 
> Diff: https://reviews.apache.org/r/69702/diff/7/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

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

[Na Li via Review Board]

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

(Updated Jan. 29, 2019, 4:15 p.m.)


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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f14cbb6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


Diff: https://reviews.apache.org/r/69702/diff/7/

Changes: https://reviews.apache.org/r/69702/diff/6-7/


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

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

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 10:14 p.m.)


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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f14cbb6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


Diff: https://reviews.apache.org/r/69702/diff/6/

Changes: https://reviews.apache.org/r/69702/diff/5-6/


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

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

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 9:03 p.m.)


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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 31e58fd 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 e504a8a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


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

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


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 6:55 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

In insecure mode, the current login user name is passed from Sentry to HMS 
server when sentry HMSFollower gets full snapshot from HMS. 

The user name should be "sentry" instead of current login user.

This issue should not happen in production because secure mode is always used. 
Insecure mode is only used in test.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 31e58fd 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 


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

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


Testing
---

Tested manually and verified the user name now is "sentry" when sentry 
HMSFollower gets notifications from HMS server


Thanks,

Na Li

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

> On Jan. 28, 2019, 1:55 a.m., kalyan kumar kalvagadda wrote:
> > Idea here is to make sure that hive client knows the details of the user 
> > who is sending the request. In this specific case, hive should know the 
> > details of the user who running sentry service. Using 
> > sentry.service.server.principal and entry.service.realm doesn’t seem 
> > correct.
> > 
> > 
> > I have a thought.
> > ```
> > public HMSClient connect() throws IOException, InterruptedException, 
> > MetaException 
> > {?
> >   UserGroupInformation clientUGI = null;
> >   if (insecure) {?   
> >   clientUGI = UserGroupInformation.getCurrentUser();?
> >   } else {?  
> >   clientUGI = 
> > UserGroupInformation.getUGIFromSubject(kerberosContext.getSubject());?
> >   }?  
> >   return new HMSClient(clientUGI.doAs(new 
> > PrivilegedExceptionAction()
> >   {?  
> >  @Override?  
> >  public HiveMetaStoreClient run() throws MetaException {? 
> >return new HiveMetaStoreClient(hiveConf);?   
> >}? 
> >   }));
> > }
> > 
> > ```
> > All you have additionally do is change the tests to run sentry server as 
> > user “sentry”. 
> > 
> > Here is the sample code. I have tested it locally.
> 
> Na Li wrote:
> HiveSimpleConnectionFactory is used by HMSFollower to get notifications 
> from HMS server. It is not used for any other purposes in Sentry.
> 
> If we following your suggestion, the user will be the login user, it 
> could be "root" for one run, and "jenkins" for another run. How to make sure 
> fetching notification from sentry works in your suggested approach?
> 
> That is why I have this solution here. Make sure the user is "sentry" in 
> insecured mode, and add "sentry" as services in HMS server.
> 
> kalyan kumar kalvagadda wrote:
> Lina, Idea is to use the UserGroupInformation.getCurrentUser(). Please 
> look at the patch i sugessted. All you have to do is perform doAs() while 
> starting the service. I have sent you details offline.
> 
> What you are suggesting will effect the users who are using sentry in non 
> secure mode. Approach that i'm usggesting will address the issues with the 
> tests and not change the behavior.
> 
> Na Li wrote:
> Kalyan, your suggestion is the current code behavior without my code 
> change.
> 
> 1) Do you agree that when sentry HMS follower gets notification, the user 
> name should be "sentry" instead of your name, or my name?
> 2) If you agree above, then your suggestion of using 
> "UserGroupInformation.getCurrentUser()" won't work because it returns current 
> login name, which is your name when you run the test, and my name if I run 
> the test, or Jenkins name name if it runs on build machine. 
> 3) When we have read authorization, HMS needs to check if the user has 
> read access to the metadata or if user is service users. 
> 3.1) If your approach is used, how do we write a test for read 
> authorization? We don't know what user name to configure as service user, or 
> give read access.
> 3.2) If my approach is used, we can add "sentry" as service user in test 
> to pass read authorization, and sentry can get notifications

Thanks! I have updated according to your suggestion: change caller of the 
HiveSimpleConnectionFactory


- Na


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


On Jan. 25, 2019, 9:07 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 25, 2019, 9:07 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-serv

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

> On Jan. 25, 2019, 9:01 p.m., kalyan kumar kalvagadda wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
> > Lines 150 (patched)
> > <https://reviews.apache.org/r/69840/diff/1/?file=2122355#file2122355line150>
> >
> > You are adding 250 msec delay. Is it good enough?
> > 
> > There are few tests that have sentryServers.size()> 1,  so we should be 
> > good even with higher delay.
> > 
> > 
> > Based on your tests if you think that this delay is good enough, i'm 
> > fine.
> 
> Na Li wrote:
> Higher delay is not necessarily good to avoid deadlock. The table 
> creation happens only when HMS followers gets notification and create table, 
> which happens after all sentry services started. So starting services with 
> delay itself does not fix the issue. What really fixes the issue is the space 
> out when HMS follower threads run. Those threads run periodically. The 
> biggest space between two threads is half of the cycle. The biggest space for 
> N threads is cycle/N.
> 
> I can make the code change to handle thread number > 2.

I have changed the code, so the first instance of sentry server does not wait 
(this avoid unnecessary wait in test). Only the following instances will wait 
for certain time to avoid creating table at the same time.


- Na


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


On Jan. 28, 2019, 4:19 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 28, 2019, 4:19 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/3/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 4:19 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

In sentry unit tests, we don't create schema before running a test. Instead, we 
use dataNucleus to create sentry tables when they are accessed. This creates 
potential deadlock when running test for Sentry HA setup.

The solution is to let the instances of sentry service start with delay. 
Specifically,
let HMS follower threads separate as far as possible, i.e., half of the 
interval.

This deadlock only exists in unit tests, and does not exist in production 
because schema is created before starting Sentry services. Therefore, there is 
no table creation after service starts.


Diffs (updated)
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


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

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


Testing
---

such deadlock does not happen with this fix.
Other unit tests passed


Thanks,

Na Li

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

> On Jan. 28, 2019, 1:55 a.m., kalyan kumar kalvagadda wrote:
> > Idea here is to make sure that hive client knows the details of the user 
> > who is sending the request. In this specific case, hive should know the 
> > details of the user who running sentry service. Using 
> > sentry.service.server.principal and entry.service.realm doesn’t seem 
> > correct.
> > 
> > 
> > I have a thought.
> > ```
> > public HMSClient connect() throws IOException, InterruptedException, 
> > MetaException 
> > {?
> >   UserGroupInformation clientUGI = null;
> >   if (insecure) {?   
> >   clientUGI = UserGroupInformation.getCurrentUser();?
> >   } else {?  
> >   clientUGI = 
> > UserGroupInformation.getUGIFromSubject(kerberosContext.getSubject());?
> >   }?  
> >   return new HMSClient(clientUGI.doAs(new 
> > PrivilegedExceptionAction()
> >   {?  
> >  @Override?  
> >  public HiveMetaStoreClient run() throws MetaException {? 
> >return new HiveMetaStoreClient(hiveConf);?   
> >}? 
> >   }));
> > }
> > 
> > ```
> > All you have additionally do is change the tests to run sentry server as 
> > user “sentry”. 
> > 
> > Here is the sample code. I have tested it locally.
> 
> Na Li wrote:
> HiveSimpleConnectionFactory is used by HMSFollower to get notifications 
> from HMS server. It is not used for any other purposes in Sentry.
> 
> If we following your suggestion, the user will be the login user, it 
> could be "root" for one run, and "jenkins" for another run. How to make sure 
> fetching notification from sentry works in your suggested approach?
> 
> That is why I have this solution here. Make sure the user is "sentry" in 
> insecured mode, and add "sentry" as services in HMS server.
> 
> kalyan kumar kalvagadda wrote:
> Lina, Idea is to use the UserGroupInformation.getCurrentUser(). Please 
> look at the patch i sugessted. All you have to do is perform doAs() while 
> starting the service. I have sent you details offline.
> 
> What you are suggesting will effect the users who are using sentry in non 
> secure mode. Approach that i'm usggesting will address the issues with the 
> tests and not change the behavior.

Kalyan, your suggestion is the current code behavior without my code change.

1) Do you agree that when sentry HMS follower gets notification, the user name 
should be "sentry" instead of your name, or my name?
2) If you agree above, then your suggestion of using 
"UserGroupInformation.getCurrentUser()" won't work because it returns current 
login name, which is your name when you run the test, and my name if I run the 
test, or Jenkins name name if it runs on build machine. 
3) When we have read authorization, HMS needs to check if the user has read 
access to the metadata or if user is service users. 
3.1) If your approach is used, how do we write a test for read authorization? 
We don't know what user name to configure as service user, or give read access.
3.2) If my approach is used, we can add "sentry" as service user in test to 
pass read authorization, and sentry can get notifications


- Na


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


On Jan. 25, 2019, 9:07 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 25, 2019, 9:07 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

> On Jan. 28, 2019, 1:55 a.m., kalyan kumar kalvagadda wrote:
> > Idea here is to make sure that hive client knows the details of the user 
> > who is sending the request. In this specific case, hive should know the 
> > details of the user who running sentry service. Using 
> > sentry.service.server.principal and entry.service.realm doesn’t seem 
> > correct.
> > 
> > 
> > I have a thought.
> > ```
> > public HMSClient connect() throws IOException, InterruptedException, 
> > MetaException 
> > {?
> >   UserGroupInformation clientUGI = null;
> >   if (insecure) {?   
> >   clientUGI = UserGroupInformation.getCurrentUser();?
> >   } else {?  
> >   clientUGI = 
> > UserGroupInformation.getUGIFromSubject(kerberosContext.getSubject());?
> >   }?  
> >   return new HMSClient(clientUGI.doAs(new 
> > PrivilegedExceptionAction()
> >   {?  
> >  @Override?  
> >  public HiveMetaStoreClient run() throws MetaException {? 
> >return new HiveMetaStoreClient(hiveConf);?   
> >}? 
> >   }));
> > }
> > 
> > ```
> > All you have additionally do is change the tests to run sentry server as 
> > user “sentry”. 
> > 
> > Here is the sample code. I have tested it locally.

HiveSimpleConnectionFactory is used by HMSFollower to get notifications from 
HMS server. It is not used for any other purposes in Sentry.

If we following your suggestion, the user will be the login user, it could be 
"root" for one run, and "jenkins" for another run. How to make sure fetching 
notification from sentry works in your suggested approach?

That is why I have this solution here. Make sure the user is "sentry" in 
insecured mode, and add "sentry" as services in HMS server.


- Na


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


On Jan. 25, 2019, 9:07 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 25, 2019, 9:07 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

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

(Updated Jan. 25, 2019, 9:46 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

In sentry unit tests, we don't create schema before running a test. Instead, we 
use dataNucleus to create sentry tables when they are accessed. This creates 
potential deadlock when running test for Sentry HA setup.

The solution is to let the instances of sentry service start with delay. 
Specifically,
let HMS follower threads separate as far as possible, i.e., half of the 
interval.

This deadlock only exists in unit tests, and does not exist in production 
because schema is created before starting Sentry services. Therefore, there is 
no table creation after service starts.


Diffs (updated)
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


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

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


Testing
---

such deadlock does not happen with this fix.
Other unit tests passed


Thanks,

Na Li

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

> On Jan. 25, 2019, 9:01 p.m., kalyan kumar kalvagadda wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
> > Lines 150 (patched)
> > <https://reviews.apache.org/r/69840/diff/1/?file=2122355#file2122355line150>
> >
> > You are adding 250 msec delay. Is it good enough?
> > 
> > There are few tests that have sentryServers.size()> 1,  so we should be 
> > good even with higher delay.
> > 
> > 
> > Based on your tests if you think that this delay is good enough, i'm 
> > fine.

Higher delay is not necessarily good to avoid deadlock. The table creation 
happens only when HMS followers gets notification and create table, which 
happens after all sentry services started. So starting services with delay 
itself does not fix the issue. What really fixes the issue is the space out 
when HMS follower threads run. Those threads run periodically. The biggest 
space between two threads is half of the cycle. The biggest space for N threads 
is cycle/N.

I can make the code change to handle thread number > 2.


- Na


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


On Jan. 25, 2019, 9:30 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 25, 2019, 9:30 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/1/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


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


Repository: sentry


Description
---

In insecure mode, the current login user name is passed from Sentry to HMS 
server when sentry HMSFollower gets full snapshot from HMS. 

The user name should be "sentry" instead of current login user.

This issue should not happen in production because secure mode is always used. 
Insecure mode is only used in test.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 31e58fd 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 


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


Testing
---

Tested manually and verified the user name now is "sentry" when sentry 
HMSFollower gets notifications from HMS server


Thanks,

Na Li

Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

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

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


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


Repository: sentry


Description
---

In sentry unit tests, we don't create schema before running a test. Instead, we 
use dataNucleus to create sentry tables when they are accessed. This creates 
potential deadlock when running test for Sentry HA setup.

The solution is to let the instances of sentry service start with delay. 
Specifically,
let HMS follower threads separate as far as possible, i.e., half of the 
interval.

This deadlock only exists in unit tests, and does not exist in production 
because schema is created before starting Sentry services. Therefore, there is 
no table creation after service starts.


Diffs
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


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


Testing
---

such deadlock does not happen with this fix.
Other unit tests passed


Thanks,

Na Li

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

[Na Li via Review Board]

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

(Updated Jan. 24, 2019, 4:15 p.m.)


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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 31e58fd 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 e504a8a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


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

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


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

Re: Sentry repository is now relocated

[Na Li]

Kakyan,

Thanks for getting this done!

Lina

On Fri, Jan 18, 2019 at 2:42 PM Kalyan Kumar Kalvagadda
 wrote:

> Hello all,
>
>
> Here are the steps to start using the new sentry repository.
>
>
> There are two ways to use the repo
>
> 1. Use Github to push. (*Current process followed*)
>
> 2. Use gitbox to push.
>
>
> If you want to use gitbox to push there are no special steps needed but if
> you want to use GitHub(*current process*), please follow below steps for
> GitHub write access.
>
>
> *For existing commitors*
>
>
>1. Visit the Gitbox Site  and link
>your GitHub account with your ASF account and setup 2fa.
>1. Follow below link to setup 2fa
>   2.
>
> https://help.github.com/articles/configuring-two-factor-authentication/
>   3. You can use any mobile for token generation. I have used "*Authy*"
>2. Wait about an hours time for things to sync up
>3. Great, you now have r/w access to both gitbox.apache.org <
>http://gitbox.apache.org> and GitHub!
>1. Follow below steps to push code to GitHub
>  1.
>
> https://help.github.com/articles/accessing-github-using-two-factor-authentication/#using-two-factor-authentication-with-the-command-line
>  2. This is a one-time process. Once it is done you can reuse
>  the access token.
>
>
> *For new contributors*
>
>
>1. Add yourself to our GitHub organization (see above) and make sure you
>are already a committer for the project.
>2. Visit the Gitbox Site  and link
>your GitHub account with your ASF account and setup 2fa.
>1. Follow below link to setup 2fa
>   2.
>
> https://help.github.com/articles/configuring-two-factor-authentication/
>   3. You can use any mobile for token generation. I have used "*Authy*"
>3. Wait about an hours time for things to sync up
>4. Great, you now have r/w access to both gitbox.apache.org <
>http://gitbox.apache.org> and GitHub!
>1. Follow below steps to push code to GitHub
>  1.
>
> https://help.github.com/articles/accessing-github-using-two-factor-authentication/#using-two-factor-authentication-with-the-command-line
>  2. This is a one-time process. Once it is done you can reuse
>  the access token.
>
> -Kalyan
>

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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 18, 2018, 11:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Dec. 18, 2018, 11:23 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
>  63f53752f5a376015dce642ca1cb59aaa1dd16ba 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  85ea6d1c06c84f89108fb1313f505dba5e324eb3 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b327e9e510483787311bf5218eac4039f04291ff 
> 
> 
> Diff: https://reviews.apache.org/r/69353/diff/4/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
Line 112 (original), 117 (patched)
<https://reviews.apache.org/r/69702/#comment297502>

1) In UserGroupInformation, if the context does not have subject, the 
getLoginUser()

  @Public
  @Evolving
  public static UserGroupInformation getCurrentUser() throws IOException {
AccessControlContext context = AccessController.getContext();
Subject subject = Subject.getSubject(context);
return subject != null && !subject.getPrincipals(User.class).isEmpty() 
? new UserGroupInformation(subject) : getLoginUser();
  }

2) removing the if (insecure) code block from the init does not work.

3) Insecure mode is usually done only for testing. In production, it is 
always secure mode.

Fixing this bug is not the focus of this jira. I change this code only to 
get tests pass. I have created SENTRY-2486 to fix this bug correctly


- Na Li


On Jan. 11, 2019, 4:55 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 11, 2019, 4:55 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
> 
> 
> 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.
> 
> This is based on changed made by Sergio at 
> https://reviews.apache.org/r/69620/, and add code to fix unstable e2e tests
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   sentry-tests/sentry-tests-hive/pom.xml 74777bb 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
>  e504a8a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
>  3c28fd0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304f 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69702/diff/3/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

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

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
Line 112 (original), 117 (patched)
<https://reviews.apache.org/r/69702/#comment297443>

If I don't make this change, in insecured mode, the user will be current 
login user. It will be hard to configure super user at HMS server to bypass 
authoriozation check at readiong metadata.

The value of realm is not important as only short user name is checked.


- Na Li


On Jan. 11, 2019, 4:55 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 11, 2019, 4:55 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
> 
> 
> 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.
> 
> This is based on changed made by Sergio at 
> https://reviews.apache.org/r/69620/, and add code to fix unstable e2e tests
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   sentry-tests/sentry-tests-hive/pom.xml 74777bb 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
>  e504a8a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
>  3c28fd0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304f 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69702/diff/3/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

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

[Na Li via Review Board]

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

(Updated Jan. 11, 2019, 4:55 a.m.)


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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 31e58fd 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 e504a8a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


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

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


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

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

[Na Li via Review Board]

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

(Updated Jan. 9, 2019, 10:39 p.m.)


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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 e504a8a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


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

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


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

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

[Na Li via Review Board]

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

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


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.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 e504a8a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


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


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


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

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

[Na Li via Review Board]

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




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
Lines 284 (patched)
<https://reviews.apache.org/r/69620/#comment297134>

if enableAuthorizingObjectStore is false, should you set 
METASTORE_RAW_STORE_IMPL to be ObjectStore? The default value is 
"org.apache.hadoop.hive.metastore.ObjectStore". Setting it here would make it 
more obvious what implementation is used.


- Na Li


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

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

[Na Li via Review Board]

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




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
Lines 284 (patched)
<https://reviews.apache.org/r/69620/#comment296942>

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.


- Na Li


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

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

[Na Li via Review Board]

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



- Na Li


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

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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 19, 2018, 3:24 p.m., Sergio Pena wrote:
> 
> ---
> 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.
> 
> 
> 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/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

[Na Li via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
Lines 67 (patched)
<https://reviews.apache.org/r/69586/#comment296489>

If the object is not a table, then this would be wrong. Is there a type 
field to check it is indeed a table?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
Lines 115 (patched)
<https://reviews.apache.org/r/69586/#comment296490>

should we keep the username case? see 
https://issues.apache.org/jira/browse/SENTRY-2432


- Na Li


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

Re: Review Request 69567: SENTRY-1679: HDFS tests configure MetastorePlugin which is gone

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 13, 2018, 9:10 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69567/
> ---
> 
> (Updated Dec. 13, 2018, 9:10 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: SENTRY-1679
> https://issues.apache.org/jira/browse/SENTRY-1679
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In TestHDFSIntegrationBase#startHiveAndMetastore() there is:
> 
> hiveConf.set("sentry.metastore.plugins", 
> "org.apache.sentry.hdfs.MetastorePlugin");
> 
> But we no longer have MetastorePlugin, so this should be removed.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  3d7fbe3 
> 
> 
> Diff: https://reviews.apache.org/r/69567/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69501: SENTRY-2466: Create generic sentry store metrics

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 3, 2018, 5:22 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69501/
> ---
> 
> (Updated Dec. 3, 2018, 5:22 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2466
> https://issues.apache.org/jira/browse/SENTRY-2466
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now there are no metrics generated for table SENTRY_GM_PRIVILEGE. Thats 
> useful information not being displayed
> 
> 
> 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/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  e48eea377 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  8cea3398f 
> 
> 
> Diff: https://reviews.apache.org/r/69501/diff/1/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestSentryPolicyStoreProcessor
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: [NOTICE] Mandatory relocation of Apache git repositories on git-wip-us.apache.org

[Na Li]

Sergio,

I prefer to move it sooner. Otherwise, we could forget when busy working on
other issues.

Thanks,

Lina

On Tue, Dec 11, 2018 at 3:11 PM Sergio Pena
 wrote:

> Hey team, the Sentry project is part of the git-wip.
> Do we want to move sooner and or later?
>
> - Sergio
>
> -- Forwarded message -
> From: Daniel Gruno 
> Date: Fri, Dec 7, 2018 at 10:53 AM
> Subject: [NOTICE] Mandatory relocation of Apache git repositories on
> git-wip-us.apache.org
> To: us...@infra.apache.org 
>
>
> [IF YOUR PROJECT DOES NOT HAVE GIT REPOSITORIES ON GIT-WIP-US PLEASE
>   DISREGARD THIS EMAIL; IT WAS MASS-MAILED TO ALL APACHE PROJECTS]
>
> Hello Apache projects,
>
> I am writing to you because you may have git repositories on the
> git-wip-us server, which is slated to be decommissioned in the coming
> months. All repositories will be moved to the new gitbox service which
> includes direct write access on github as well as the standard ASF
> commit access via gitbox.apache.org.
>
> ## Why this move? ##
> The move comes as a result of retiring the git-wip service, as the
> hardware it runs on is longing for retirement. In lieu of this, we
> have decided to consolidate the two services (git-wip and gitbox), to
> ease the management of our repository systems and future-proof the
> underlying hardware. The move is fully automated, and ideally, nothing
> will change in your workflow other than added features and access to
> GitHub.
>
> ## Timeframe for relocation ##
> Initially, we are asking that projects voluntarily request to move
> their repositories to gitbox, hence this email. The voluntary
> timeframe is between now and January 9th 2019, during which projects
> are free to either move over to gitbox or stay put on git-wip. After
> this phase, we will be requiring the remaining projects to move within
> one month, after which we will move the remaining projects over.
>
> To have your project moved in this initial phase, you will need:
>
> - Consensus in the project (documented via the mailing list)
> - File a JIRA ticket with INFRA to voluntarily move your project repos
>over to gitbox (as stated, this is highly automated and will take
>between a minute and an hour, depending on the size and number of
>your repositories)
>
> To sum up the preliminary timeline;
>
> - December 9th 2018 -> January 9th 2019: Voluntary (coordinated)
>relocation
> - January 9th -> February 6th: Mandated (coordinated) relocation
> - February 7th: All remaining repositories are mass migrated.
>
> This timeline may change to accommodate various scenarios.
>
> ## Using GitHub with ASF repositories ##
> When your project has moved, you are free to use either the ASF
> repository system (gitbox.apache.org) OR GitHub for your development
> and code pushes. To be able to use GitHub, please follow the primer
> at: https://reference.apache.org/committer/github
>
>
> We appreciate your understanding of this issue, and hope that your
> project can coordinate voluntarily moving your repositories in a
> timely manner.
>
> All settings, such as commit mail targets, issue linking, PR
> notification schemes etc will automatically be migrated to gitbox as
> well.
>
> With regards, Daniel on behalf of ASF Infra.
>
> PS:For inquiries, please reply to us...@infra.apache.org, not your
> project's dev list :-).
>

Re: Review Request 69536: SENTRY-2471: Table rename should sync Sentry privilege even without location information

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
Lines 418 (patched)
<https://reviews.apache.org/r/69536/#comment296098>

Can you make comment more explicit on path info is not complete?


- Na Li


On Dec. 10, 2018, 2:21 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69536/
> ---
> 
> (Updated Dec. 10, 2018, 2:21 a.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2471: Table rename should sync Sentry privilege even without location 
> information
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  7b7d0e1eb7ea1f17dea622e51ccc557e0b76fbff 
> 
> 
> Diff: https://reviews.apache.org/r/69536/diff/2/
> 
> 
> Testing
> ---
> 
> existing unit tests
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


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 69536: SENTRY-2471: Table rename should sync Sentry privilege even without location information

[Na Li via Review Board]

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



can you add test case to cover the issue you want to fix? There is no path 
info, but if the table name is changed, the privilege is moved to table of new 
name?

- Na Li


On Dec. 10, 2018, 2:21 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69536/
> ---
> 
> (Updated Dec. 10, 2018, 2:21 a.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2471: Table rename should sync Sentry privilege even without location 
> information
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  7b7d0e1eb7ea1f17dea622e51ccc557e0b76fbff 
> 
> 
> Diff: https://reviews.apache.org/r/69536/diff/2/
> 
> 
> Testing
> ---
> 
> existing unit tests
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

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

[Na Li via Review Board]

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



Can you add more test cases to cover the cases that mutiple entries of same 
path_name, but different authzObj. And make sure you don't delete entries of 
different authzObj value.

- Na Li


On Dec. 10, 2018, 2:11 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 10, 2018, 2:11 a.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
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
> 
> 
> Diff: https://reviews.apache.org/r/69530/diff/3/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestNotificationProcessor
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

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

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3454 (original), 3451 (patched)
<https://reviews.apache.org/r/69530/#comment296073>

Why do you remove the input of "currentSnapshotID"? There could be multiple 
snapshots, and we need to make sure to remove the one in the latest Snapsho



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3455 (original), 3452 (patched)
<https://reviews.apache.org/r/69530/#comment296076>

if it possible that there are multiple instances in table AUTHZ_PATH, they 
are of the same value of "PATH_NAME", but belong to different  "authzObj"?

If that is the case, you delect more entries than you need to. Basically, 
you delete all entries matching input "paths" regardless the value of 
"authzObj". That is not right.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3551 (original), 3539 (patched)
<https://reviews.apache.org/r/69530/#comment296074>

why do you remove input "currentSnapshotID"



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3739 (patched)
<https://reviews.apache.org/r/69530/#comment296075>

path is a collection. Can you change the input parameter name to reflect 
that?


- Na Li


On Dec. 10, 2018, 2:11 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 10, 2018, 2:11 a.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
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
> 
> 
> Diff: https://reviews.apache.org/r/69530/diff/3/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestNotificationProcessor
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

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

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3932 (patched)
<https://reviews.apache.org/r/69353/#comment296043>

If column name is not null, should add that in filter



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3943 (patched)
<https://reviews.apache.org/r/69353/#comment296044>

You don't fetch users of that privilege. Do you want to return that? If the 
caller does not want it, it can be dropped. Or let a input indicating if we 
want to get users as well


- Na Li


On Dec. 7, 2018, 12:05 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Dec. 7, 2018, 12:05 a.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/2/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API 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.

[Na Li via Review Board]

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



typo: "sentrys tore" should be "sentry store"

- Na Li


On Dec. 7, 2018, 12:05 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Dec. 7, 2018, 12:05 a.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/2/
> 
> 
> 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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


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 69351: SENTRY-2458: Split web service from server service modules

[Na Li via Review Board]

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




sentry-dist/src/license/THIRD-PARTY.properties
Line 34 (original)
<https://reviews.apache.org/r/69351/#comment295816>

is this change intentially?



sentry-dist/src/license/THIRD-PARTY.properties
Line 37 (original), 37 (patched)
<https://reviews.apache.org/r/69351/#comment295815>

is this change intentially?


- Na Li


On Nov. 29, 2018, 7:58 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69351/
> ---
> 
> (Updated Nov. 29, 2018, 7:58 p.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2458
> https://issues.apache.org/jira/browse/SENTRY-2458
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2458: Split web service from server service modules
> 
> In order to additional modules to be added to Sentry there needs to be a 
> separation of some of the features used by service-server into other modules.
> 
> This will allow the web server to be pulled out from the base server and 
> allow for additional modules to be able to add web services and functionality 
> to the web interface without depending up the whole server module.
> 
> It will use the SPI to dynamically include servlets and content from other 
> modules.
> 
> It creates a sentry-service-providers to allow for Server and sub modules SPI 
> providers to be defined externally and not depended on directly.
> 
> 
> Diffs
> -
> 
>   pom.xml 46ca38e9a 
>   sentry-dist/pom.xml 62558d2e0 
>   sentry-dist/src/license/THIRD-PARTY.properties a1084db69 
>   sentry-dist/src/main/assembly/bin.xml 986530c55 
>   sentry-provider/sentry-provider-db/pom.xml df569474a 
>   sentry-service/pom.xml e653189eb 
>   sentry-service/sentry-service-providers/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/AttributeDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/FilterDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/ServletDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProviderFactory.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceSpi.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/resources/META-INF/services/org.apache.sentry.spi.Spi
>  PRE-CREATION 
>   sentry-service/sentry-service-server/pom.xml 44540ad5d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
>  862548745 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/LogLevelServlet.java
>  af81d6fce 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/PubSubServlet.java
>  8da35f10f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAdminServlet.java
>  5dc6cd6c4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAuthFilter.java
>  23121ecf5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryServiceWebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryWebServer.java
>  befe6c3ed 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934df 
>   
> sentry-service/sentry-service-server/src/main/resources/META-INF/services/org.apache.sentry.server.provider.webservice.WebServiceProviderFactory
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/webapp/css/bootstrap-theme

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

[Na Li via Review Board]

> On Nov. 29, 2018, 6:44 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Lines 176 (patched)
> > <https://reviews.apache.org/r/69087/diff/10/?file=2111053#file2111053line182>
> >
> > do you want to clear pathsToPersist after persisting them? Otherwise, 
> > next time it is called, those paths will be added again
> 
> kalyan kumar kalvagadda wrote:
> Persisting is the last thing that we do on an instance of 
> MAuthzPathsMapping. This shouldn't be an issue.

what happens if makePersistent is called, then more paths are added, then 
makePersistent is called again? If you don't clean pathsToPersist, the old 
paths (already persisted) will be persisted again. It is a bug, and need to be 
fixed.


- Na


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


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 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 176 (patched)
<https://reviews.apache.org/r/69087/#comment295778>

do you want to clear pathsToPersist after persisting them? Otherwise, next 
time it is called, those paths will be added again


- Na Li


On Nov. 29, 2018, 6:32 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, 6:32 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/10/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 78 (original), 88 (patched)
<https://reviews.apache.org/r/69087/#comment295777>

this seems to be a bug. If paths contains path_1, and caller adds path_2, 
this change results in paths contains only path_2, but lost path_1.


- Na Li


On Nov. 21, 2018, 7:48 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 21, 2018, 7:48 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/NotificationProcessor.java
>  ab262d0a849852bd95d88dd099dc6bf187715cad 
>   
> 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/8/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69351: SENTRY-2458: Split web service from server service modules

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 27, 2018, 3:32 a.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69351/
> ---
> 
> (Updated Nov. 27, 2018, 3:32 a.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2458
> https://issues.apache.org/jira/browse/SENTRY-2458
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2458: Split web service from server service modules
> 
> In order to additional modules to be added to Sentry there needs to be a 
> separation of some of the features used by service-server into other modules.
> 
> This will allow the web server to be pulled out from the base server and 
> allow for additional modules to be able to add web services and functionality 
> to the web interface without depending up the whole server module.
> 
> It will use the SPI to dynamically include servlets and content from other 
> modules.
> 
> It creates a sentry-service-providers to allow for Server and sub modules SPI 
> providers to be defined externally and not depended on directly.
> 
> 
> Diffs
> -
> 
>   pom.xml 46ca38e9a 
>   sentry-dist/pom.xml 62558d2e0 
>   sentry-dist/src/main/assembly/bin.xml 986530c55 
>   sentry-provider/sentry-provider-db/pom.xml df569474a 
>   sentry-service/pom.xml e653189eb 
>   sentry-service/sentry-service-providers/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/AttributeDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/FilterDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/ServletDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProviderFactory.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceSpi.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/resources/META-INF/services/org.apache.sentry.spi.Spi
>  PRE-CREATION 
>   sentry-service/sentry-service-server/pom.xml 44540ad5d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
>  862548745 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/LogLevelServlet.java
>  af81d6fce 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/PubSubServlet.java
>  8da35f10f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAdminServlet.java
>  5dc6cd6c4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAuthFilter.java
>  23121ecf5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryServiceWebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryWebServer.java
>  befe6c3ed 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934df 
>   
> sentry-service/sentry-service-server/src/main/resources/META-INF/services/org.apache.sentry.server.provider.webservice.WebServiceProviderFactory
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/webapp/css/bootstrap-theme.min.css
>  c31428b07 
>   sentry-service/sentry-service-server/src/main/webapp/css/bootstrap.min.css 
> a553c4f5e 
>   sentry-service/sentry-service-server/src/main/webapp/css/sentry.css  
>   sentry-service/sentry-service-server/src/main/webapp/sentry.png  
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/T

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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 27, 2018, 9:34 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 27, 2018, 9:34 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/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

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

[Na Li via Review Board]

> On Nov. 26, 2018, 7:44 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
> > Line 37 (original), 37 (patched)
> > <https://reviews.apache.org/r/69448/diff/1/?file=2110388#file2110388line37>
> >
> > should this be volatile  or atomic?
> 
> Arjun Mishra wrote:
> Lina, can you explain why adding volatile here is needed? For each Kafka 
> broker there is a single KafkaAuthzBinding instance. So 
> UpdatableCache#startUpdateThread will be called once by a KafkaBroker. This 
> means there will be a reloadData thread for each Kafka broker.

Is it possible for more than one threads to access that single 
KafkaAuthzBinding instance? If so, we should define "initialized" as volatile 
to handle concurrency correctly. Otherwise, you don't need to do so


- Na


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


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