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

[kalyan kumar kalvagadda via Review Board]

> On Sept. 17, 2020, 3:44 p.m., Na Li wrote:
> > Ship It!

Thanks for the review. Will merge the change after the unit tests are green.


- kalyan kumar


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


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

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

[kalyan kumar kalvagadda via Review Board]

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

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 = t
 est_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(FullUpdateInitia
 lizer.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

[kalyan kumar kalvagadda 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.
> 
> Na Li wrote:
> 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.

Can you explain the deployment?


- kalyan kumar


---
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 72704: SENTRY-2422: HMS synchronization is causing multiple entries of the same ID in SENTRY_HMS_NOTIFICATION_ID

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Change looks good, you need to understand that this reduces the chance of 
having dplicate entries but does not guarentee it.

- kalyan kumar kalvagadda


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 72704: SENTRY-2422: HMS synchronization is causing multiple entries of the same ID in SENTRY_HMS_NOTIFICATION_ID

[kalyan kumar kalvagadda via Review Board]

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



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


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

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

[kalyan kumar kalvagadda via Review Board]

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

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

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

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Dec. 22, 2019, 9:45 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> ---
> 
> (Updated Dec. 22, 2019, 9:45 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
>  8f45c60 
>   
> 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/SimpleFilteredPrivilegeCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java
>  5de3135 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java
>  f2f735b 
>   
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java
>  891c1d9 
>   
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java
>  d50a0bc 
>   
> 

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

[kalyan kumar kalvagadda via Review Board]

> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
> > Lines 97 (patched)
> > 
> >
> > do we need copyOf?

How about using 
Collections.unmodifiableSet(filteredPrivilegeCache.listPrivilegeObjects(groups, 
users, roleSet, authorizableHierarchy)));
This avoids creating new copy of the collection for each call. It still makes 
sure that the caller will not be able to modify the list.
Using Collections.unmodifiableList creates a wrapper around your List. if the 
underlying list changes, so does your unmodifiableList's view.

Creating copy is not efficient. It's a more expensive computation and consumes 
more memor.


- kalyan kumar


---
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-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/SimpleFilteredPrivilegeCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java
>  5de3135 
>   
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
>  PRE-CREATION 
>   
> 

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

[kalyan kumar kalvagadda via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
Lines 233-236 (patched)


Did you see any issue with out this change?



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
Line 46 (original)


I agree.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
Lines 75-78 (original), 76-84 (patched)


Why should be do this up-casting?

If the below API is not removed from Privilge cache we would need this 
special check.

  Set listPrivileges(Set groups, Set users, 
ActiveRoleSet roleSet,
  Authorizable... authorizationhierarchy);
  
  
  Is there any reason to remove this API. This might even break Impala 
which implements this interface.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 170 (patched)


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.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 174 (patched)


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

+1 on vehang's comment.



sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Lines 217-225 (patched)


Why are you converting the privilge object to string and performening the 
check?

Something like

 private boolean hasOnlyServerPrivilege(Privilege privObj) {
if(privObj.getParts().size() == 1 && 
privObj.getParts().get(0).getKey().equalsIgnoreCase("server")) {
  return privObj.getParts().get(0).getValue().endsWith("+");
}
return false;
  }



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java
Lines 111-151 (patched)


Can you avoid code duplication?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
Lines 137-150 (patched)


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.


- kalyan kumar kalvagadda


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

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

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Dec. 13, 2019, 4:38 p.m., Na Li wrote:
> 
> ---
> 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
> -
> 
>   
> 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/
> 
> 
> 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

[kalyan kumar kalvagadda via Review Board]

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



Code change looks good. Please add unit tests to cover the same.

- kalyan kumar kalvagadda


On Dec. 11, 2019, 9:17 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71901/
> ---
> 
> (Updated Dec. 11, 2019, 9:17 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
> -
> 
>   
> 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 71532: SENTRY-2533: The UDF in_file should be blacked default

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Code change looks good.

- kalyan kumar kalvagadda


On Sept. 25, 2019, 6:54 a.m., Wenchao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71532/
> ---
> 
> (Updated Sept. 25, 2019, 6:54 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: SENTRY-2533
> https://issues.apache.org/jira/browse/SENTRY-2533
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> HIVE-20420(CVE-2018-11777) introduced a fallback authorizer factory which 
> disallowed some builtin UDFs such as java_method, reflect, reflect2 and 
> in_file. But Sentry does not black in_file up to now, so a malicious user can 
> use in_file in SQL queries to detect some specific files on the HS2 host, or 
> to detect whether a specific file has specific content. in_file should be 
> added to HIVE_UDF_BLACK_LIST.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  5c433290972d5ffc52ef342678b6b11e48f28cef 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
>  c6e14a56004a934a025cc7abb2fe6646e5d36768 
> 
> 
> Diff: https://reviews.apache.org/r/71532/diff/1/
> 
> 
> Testing
> ---
> 
> Added a unit test to test if it is properly blacked.
> 
> 
> Thanks,
> 
> Wenchao Li
> 
>

Re: Review Request 71039: SENTRY-2528: Format exception when fetching a full snapshot

[kalyan kumar kalvagadda via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
Line 548 (original), 548 (patched)


Consider changing it as below

percentageDatabasesFetched = totalNumberOfDatabases > 0? 
((totalNumberOfDatabasesFetched*100)/totalNumberOfDatabases):0;

This way you can avaoid the explicit casting. This applies below statements 
as well.


- kalyan kumar kalvagadda


On July 9, 2019, 3 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71039/
> ---
> 
> (Updated July 9, 2019, 3 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-2528
> https://issues.apache.org/jira/browse/SENTRY-2528
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When fetching a full snapshot we get the below error. This is a regression of 
> SENTRY-2301
> 
> 2019-07-07 23:07:39,677 ERROR 
> org.apache.sentry.service.thrift.SentryHMSClient: Snapshot created failed
> java.util.IllegalFormatConversionException: f != java.lang.Long
> at 
> java.util.Formatter$FormatSpecifier.failConversion(Formatter.java:4302)
> at java.util.Formatter$FormatSpecifier.printFloat(Formatter.java:2806)
> at java.util.Formatter$FormatSpecifier.print(Formatter.java:2753)
> at java.util.Formatter.format(Formatter.java:2520)
> at java.util.Formatter.format(Formatter.java:2455)
> at java.lang.String.format(String.java:2940)
> at 
> org.apache.sentry.service.thrift.FullUpdateInitializer.getFullHMSSnapshot(FullUpdateInitializer.java:552)
> at 
> org.apache.sentry.service.thrift.SentryHMSClient.fetchFullUpdate(SentryHMSClient.java:244)
> at 
> org.apache.sentry.service.thrift.SentryHMSClient.getFullSnapshot(SentryHMSClient.java:147)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.createFullSnapshot(HMSFollower.java:409)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.syncupWithHms(HMSFollower.java:237)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.run(HMSFollower.java:198)
> at 
> java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
> at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
> at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
> at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
> 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)
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  d4bca42faea51d87164568c23bd7849c56450f32 
> 
> 
> Diff: https://reviews.apache.org/r/71039/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 70337: SENTRY-2511: Debug level logging on HMSPaths significantly affects performance

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Looks good.

- kalyan kumar kalvagadda


On March 29, 2019, 1:01 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70337/
> ---
> 
> (Updated March 29, 2019, 1:01 a.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-2511
> https://issues.apache.org/jira/browse/SENTRY-2511
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Newer logging changes were made to HMSPath to help identify the corrupt 
> cache. However when there are large number of partitions logging changes made 
> makes the processing or creation of an update very slow
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  c49e24427 
> 
> 
> Diff: https://reviews.apache.org/r/70337/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 70004: SENTRY-2496 Support multi-field attribute based document level controls for Solr

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Feb. 21, 2019, 7:45 p.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70004/
> ---
> 
> (Updated Feb. 21, 2019, 7:45 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This is an improvement request to cover enhanced document level security for 
> the Solr document level controls, specifically to cover:
> 
> Security controls against multiple fields
> Filters based on user attributes as well as just Sentry roles
> Different security predicates (AND, LessThan, GreaterThan, OR)
> Pluggable user attribute source ahead of Sentry enhancements.
> Sample LDAP user attribute source
> The ambition is this will be a precursor to full complex predicate support 
> being served by Sentry ABAC roadmap.
> 
> 
> Diffs
> -
> 
>   sentry-solr/solr-sentry-handlers/pom.xml 621d8325 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/CachingUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/FieldToAttributeMapping.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SolrAttrBasedFilter.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/CachingUserAttributeSourceTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/LdapRegexTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/MockUserAttributeSource.java
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/pom.xml 7c28bda5 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/SolrSentryServiceTestBase.java
>  09f095a3 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestAbacOperations.java
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/ldap/ldap.ldiff 
> PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/ldap/ldap.schema 
> PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_abac/conf/enumsConfig.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_abac/conf/schema.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_abac/conf/solrconfig.xml
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70004/diff/2/
> 
> 
> Testing
> ---
> 
> Added unit tests to cover specific areas and also TestAbacOperations.java 
> which tests full integration with a mocked LDAP server against a live 
> SolrCloud.
> 
> Addiitonally I've run manual tests against a live Solr cluster with this 
> plugin deployed against Active Directory.
> 
> 
> Thanks,
> 
> Tristan Stevens
> 
>

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Feb. 23, 2019, 5:28 a.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 (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 1dfa1d9aafbd482903e4bb935509a76768e02b58 
  
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/2/

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


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

[kalyan kumar kalvagadda via Review Board]

> On Feb. 22, 2019, 8:58 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Lines 507 (patched)
> > 
> >
> > should this be removed if it has no authz obj?
> > 
> > Why change its type to DIR?

one entry object is created for every directoy in the path of file. Some of the 
directories may not be associated with any object. In those cases Entry is 
marked as DIR. 
In this case if Entry is not associated with any object it should be marked as 
DIR. Previously it was marked as directory when when it was associated with one 
or more objects.


- kalyan kumar


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


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

[kalyan kumar kalvagadda via Review Board]

> On Feb. 22, 2019, 8:46 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Line 689 (original), 694 (patched)
> > 
> >
> > 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?

If we use the updated Entry to find/delete it from the set it will NOT be 
found/delted."


Yes, there will be performence impact when the collection of Entries is tool 
authzObjToEntries when the updates from sentry are processed.


- kalyan kumar


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


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

[kalyan kumar kalvagadda via Review Board]

> On Feb. 22, 2019, 6:48 p.m., Haley Reeve wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Line 689 (original), 694 (patched)
> > 
> >
> > Why the change from sets to lists? Was there some kind of duplicate 
> > entries that were getting lost when using a set? (Just asking for my own 
> > curiosity since I'm not super familiar with this code.)

We were using HashSet before. When a element is inserted into hashset there is 
hash value calculated and stored for that element. This hash value is used for 
future lookups.

Entry's that were inserted into hashset are updated later over time. There are 
no reference older Entry. If we use the updated Entry to find/delete it from 
the set it will be found/delted. Reason for that is that hash value of Entry 
which is updated does not match hash value calculated when it was inserted. As 
a result, code we had was never able to remove the entries from 
authzObjToEntries. That is the reason i have changed it to a list.


- kalyan kumar


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


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

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

[kalyan kumar kalvagadda via Review Board]

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

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 70008: SENTRY-2500: CREATE on server does not provide HMS server side read authorization for get_all_tables(database_name)

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Feb. 20, 2019, 9:39 p.m., Na Li wrote:
> 
> ---
> 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
> -
> 
>   
> 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/
> 
> 
> Testing
> ---
> 
> existing tests for metastore, and add two new tests for reading database and 
> tables.
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 70004: SENTRY-2496 Support multi-field attribute based document level controls for Solr

[kalyan kumar kalvagadda via Review Board]

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




sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/CachingUserAttributeSource.java
Lines 33 (patched)


Please add java doc for this class.



sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/FieldToAttributeMapping.java
Lines 26 (patched)


Please add java doc for this class.



sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SolrAttrBasedFilter.java
Lines 114-119 (patched)


How does CachingUserAttributeSource work with LdapUserAttributeSource.

Let's say cache is enabled and source is ldap then there will be two 
cache's, right as there is a cache in LdapUserAttributeSource.


- kalyan kumar kalvagadda


On Feb. 18, 2019, 2:36 p.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70004/
> ---
> 
> (Updated Feb. 18, 2019, 2:36 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This is an improvement request to cover enhanced document level security for 
> the Solr document level controls, specifically to cover:
> 
> Security controls against multiple fields
> Filters based on user attributes as well as just Sentry roles
> Different security predicates (AND, LessThan, GreaterThan, OR)
> Pluggable user attribute source ahead of Sentry enhancements.
> Sample LDAP user attribute source
> The ambition is this will be a precursor to full complex predicate support 
> being served by Sentry ABAC roadmap.
> 
> 
> Diffs
> -
> 
>   sentry-solr/solr-sentry-handlers/pom.xml 621d8325 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/CachingUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/FieldToAttributeMapping.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SolrAttrBasedFilter.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/CachingUserAttributeSourceTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/LdapRegexTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/MockUserAttributeSource.java
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/pom.xml 7c28bda5 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/SolrSentryServiceTestBase.java
>  09f095a3 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestAbacOperations.java
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/ldap/ldap.ldiff 
> PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/ldap/ldap.schema 
> PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_abac/conf/enumsConfig.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_abac/conf/schema.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_abac/conf/solrconfig.xml
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70004/diff/1/
> 
> 
> Testing
> ---
> 
> Added unit tests to cover specific areas and also TestAbacOperations.java 
> which tests full integration with a mocked LDAP server against a live 
> SolrCloud.
> 
> Addiitonally I've run manual tests against a live Solr cluster with this 
> plugin deployed against Active Directory.
> 
> 
> Thanks,
> 
> Tristan Stevens
> 
>

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

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


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

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

[kalyan kumar kalvagadda via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Lines 479-506 (patched)


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?


- kalyan kumar kalvagadda


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

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

[kalyan kumar kalvagadda via Review Board]

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




sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
Lines 58-78 (original)


Same comment as above.


- kalyan kumar kalvagadda


On Feb. 19, 2019, 9:56 p.m., Na Li wrote:
> 
> ---
> 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
> -
> 
>   
> 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/
> 
> 
> 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)

[kalyan kumar kalvagadda via Review Board]

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




sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
Lines 83-102 (original)


I prefer having the older code. Reusing the privileges defined in 
MetastoreAuthzObjectFilter might not be good option. Here is why

Currently test code has defined LIST_DATABASES_PRIVILEGES and 
LIST_TABLES_PRIVILEGES seperately. Some time later some one change the 
priviliege definations in MetastoreAuthzObjectFilter tests would fail because 
of they are defined seperatly. It's ont way to find something changed. 

If test code used he same definations there will be chance that issues will 
not be exposed by tests.

I understand multiple tests has the same code. You can normalize that part 
if you want but resuing the definations in MetastoreAuthzObjectFilter might not 
be good idea.


- kalyan kumar kalvagadda


On Feb. 19, 2019, 9:56 p.m., Na Li wrote:
> 
> ---
> 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
> -
> 
>   
> 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/
> 
> 
> Testing
> ---
> 
> existing tests for metastore, and add two new tests for reading database and 
> tables.
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69975: SENTRY-2495: Support Conjunctive Matching in Solr QueryDocAuthorizationComponent

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Feb. 15, 2019, 4:54 p.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69975/
> ---
> 
> (Updated Feb. 15, 2019, 4:54 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adds a conjunctive match capabiltiy to the existing 
> QueryDocAuthorizationComponent (which applies document level security against 
> Sentry roles)
> 
> 
> Diffs
> -
> 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/DocAuthorizationComponent.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java
>  9da3d6e1 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SubsetQueryPlugin.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/SubsetQueryTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/schema-docValuesSubsetMatch.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetmatchcomponent.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetquery.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig.snippet.randomindexconfig.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestCase.java
>  3d4d555f 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/DocLevelGenerator.java
>  40cc153e 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/SolrSentryServiceTestBase.java
>  e1f789cb 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestDocLevelOperations.java
>  7834f339 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSubsetQueryOperations.java
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/log4j.properties d9418167 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match/conf/schema.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match/conf/solrconfig.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match_missing_false/conf/schema.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/resources/solr/configsets/cloud-minimal_subset_match_missing_false/conf/solrconfig.xml
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69975/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added for SubsetQueryPlugin
> End-to-end tests added under sentry-tests/senty-tests-solr for full 
> SolrCloud+Sentry Service using the new QueryDocAuthorizationComponent in 
> conjunctive match mode
> 
> 
> Thanks,
> 
> Tristan Stevens
> 
>

Re: Review Request 69619: SENTRY-2482: Sentry Solr to support multi-attribute document level security

[kalyan kumar kalvagadda via Review Board]

> On Feb. 12, 2019, 8:38 p.m., kalyan kumar kalvagadda wrote:
> > Tristan Stevens,
> > 
> > This patch has two enhancements
> > 1.  Subset Match Filtering/
> > 2.  User Attribute Filter
> > 
> > Having multiple enhacements in single commit is not easy to understand. Can 
> > you seperate them to two different patches?
> 
> Tristan Stevens wrote:
> Hi Kalyan,
> I understand the concern. The problem we have is that both enhancements 
> depend on the SubsetQueryPlugin, which in itself doesn't merit a JIRA on its 
> own. Also, I'm not sure how do submit a review board for one patch built on 
> another.
> 
> To break it down, please consider:
> 
> 1. SubsetQueryPlugin.java which used by both 
> QueryDocAuthorizationComponent.java and SolrAttrBasedFilter.java.
> 2. QueryDocAuthorizationComponent.java provides the first enhancement, 
> extending DocAuthorizationComponent.java
> 3. All other classes under src/main are related to 
> SolrAttrBasedFilter.java.

Tristan,

You don't have to submit both the patches at the same time. You can sumbit one 
first, i will reiview it and commit it so that you can submit another one after 
that.
I will not out delay in the review process.


- kalyan kumar


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


On Jan. 15, 2019, 9:49 p.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69619/
> ---
> 
> (Updated Jan. 15, 2019, 9:49 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This is an improvement request to cover enhanced document level security for 
> the Solr document level controls, specifically to cover:
> 
> - Security controls against multiple fields
> - Filters based on user attributes as well as just Sentry roles
> - Different security predicates (AND, LessThan, GreaterThan - in addition to 
> the currently supported OR)
> - Pluggable user attribute source ahead of Sentry enhancements.
> - Sample LDAP user attribute source
> 
> The ambition is this will be a precursor to full complex predicate support 
> being served by Sentry ABAC roadmap.
> 
> 
> Diffs
> -
> 
>   sentry-solr/solr-sentry-handlers/pom.xml 621d8325 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/CachingUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/DocAuthorizationComponent.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/FieldToAttributeMapping.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java
>  9da3d6e1 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SolrAttrBasedFilter.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SubsetQueryPlugin.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/CachingUserAttributeSourceTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/LdapRegexTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/MockUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/SubsetQueryTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/schema-docValuesSubsetMatch.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetmatchcomponent.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetquery.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig.snippet.randomindexconfig.xml
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/pom.xml 7c28bda5 
>   
> 

Re: Review Request 69619: SENTRY-2482: Sentry Solr to support multi-attribute document level security

[kalyan kumar kalvagadda via Review Board]

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



Tristan Stevens,

This patch has two enhancements
1.  Subset Match Filtering/
2.  User Attribute Filter

Having multiple enhacements in single commit is not easy to understand. Can you 
seperate them to two different patches?

- kalyan kumar kalvagadda


On Jan. 15, 2019, 9:49 p.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69619/
> ---
> 
> (Updated Jan. 15, 2019, 9:49 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This is an improvement request to cover enhanced document level security for 
> the Solr document level controls, specifically to cover:
> 
> - Security controls against multiple fields
> - Filters based on user attributes as well as just Sentry roles
> - Different security predicates (AND, LessThan, GreaterThan - in addition to 
> the currently supported OR)
> - Pluggable user attribute source ahead of Sentry enhancements.
> - Sample LDAP user attribute source
> 
> The ambition is this will be a precursor to full complex predicate support 
> being served by Sentry ABAC roadmap.
> 
> 
> Diffs
> -
> 
>   sentry-solr/solr-sentry-handlers/pom.xml 621d8325 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/CachingUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/DocAuthorizationComponent.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/FieldToAttributeMapping.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/LdapUserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java
>  9da3d6e1 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SolrAttrBasedFilter.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SubsetQueryPlugin.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/UserAttributeSourceParams.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/CachingUserAttributeSourceTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/LdapRegexTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/MockUserAttributeSource.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/java/org/apache/solr/handler/component/SubsetQueryTest.java
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/schema-docValuesSubsetMatch.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetmatchcomponent.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig-subsetquery.xml
>  PRE-CREATION 
>   
> sentry-solr/solr-sentry-handlers/src/test/resources/solr/collection1/solrconfig.snippet.randomindexconfig.xml
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/pom.xml 7c28bda5 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestCase.java
>  3d4d555f 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/DocLevelGenerator.java
>  40cc153e 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/SolrSentryServiceTestBase.java
>  e1f789cb 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestAbacOperations.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestDocLevelOperations.java
>  7834f339 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestSubsetQueryOperations.java
>  PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/ldap/ldap.ldiff 
> PRE-CREATION 
>   sentry-tests/sentry-tests-solr/src/test/resources/ldap/ldap.schema 
> PRE-CREATION 
>   

Re: Review Request 69941: SENTRY-2494: Fix TestRollingFileWithoutDeleteAppender test case testFileNamePattern

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Feb. 11, 2019, 5:30 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69941/
> ---
> 
> (Updated Feb. 11, 2019, 5:30 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-2494
> https://issues.apache.org/jira/browse/SENTRY-2494
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The log size is set to 10 bytes. However if the message size is 15 bytes, it 
> creates a 15, 15 and 0 byte file ( which is sometimes flaky)
> 
> Explanation:
> Before we logged a string that was at 15 bytes each. The assumption was 
> Logger would split that across 2 files but it never did that. It would put 15 
> bytes of line on one file.
> 
> Previously we had 2 log statements:
> debug."123456789012345"; 
> debug."123456789012345";
> 
> The file being created was "123456789012345", "123456789012345", "" (LAST ONE 
> empty)
> 
> as opposed to "1234567890", "1234512345", "6789012345"
> 
> The above output would be flaky because LOGGER.appender did not handle a LONG 
> string properly. It would sometimes generate two files with 
> "123456789012345", "" (LAST ONE empty)
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/appender/TestRollingFileWithoutDeleteAppender.java
>  5db15a3d0e78a071ec10c9fba4048c95f2f0cc89 
> 
> 
> Diff: https://reviews.apache.org/r/69941/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




- kalyan kumar kalvagadda


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

> On Feb. 5, 2019, 11:13 p.m., Arjun Mishra wrote:
> > Left last bits of comments. Please fix them and shit it

Ok. I will fix them and push the changes.


- kalyan kumar


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


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

> On Feb. 5, 2019, 8:25 p.m., kalyan kumar kalvagadda wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 124 (original), 124 (patched)
> > 
> >
> > why? What is missing new the one?
> > Looking at the previous one can not know what numbers mean. With new 
> > one, you can see what is the perm sequence number ,path sequence number and 
> > path image number.
> 
> Arjun Mishra wrote:
> There are 3 {} but 4 arguments. You need one more {} for perm updates 
> size. Also nit pick: Can you change NUmber to Number

I see, Will fix.


- kalyan kumar


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


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

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




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


why? What is missing new the one?
Looking at the previous one can not know what numbers mean. With new one, 
you can see what is the perm sequence number ,path sequence number and path 
image number.


- kalyan kumar kalvagadda


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

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

(Updated Feb. 5, 2019, 12:01 a.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.


Changes
---

Addressed review comments.


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


Repository: sentry


Description
---

Logging will help us identify below
1. Track the updates from Sentry
2. What the path update is?
3. What the perm update is?
4. How the path change is effecting the exizting cache and what is the result 
of the update?
5. How the perm change is effecting the exizting cache and what is the result 
of the update?
6. Changes to the path tree stored as part of the cache.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
 db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
 bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
 c967f4e2ee0a811e04362aef452b1cfddfd973f3 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 9dae03562aff82574dcda5339df415193655d5a2 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 5726c0e834e6e5ad47f47f3337d9317ee1366955 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
 9115697088cab8fd1732086963c7c3f982069380 


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

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


Testing
---

As there is no functiuonal change I have not added any tests. I made sure that 
existing tests pass.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

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

(Updated Feb. 1, 2019, 10:22 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.


Changes
---

addressed review comments.


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


Repository: sentry


Description
---

Logging will help us identify below
1. Track the updates from Sentry
2. What the path update is?
3. What the perm update is?
4. How the path change is effecting the exizting cache and what is the result 
of the update?
5. How the perm change is effecting the exizting cache and what is the result 
of the update?
6. Changes to the path tree stored as part of the cache.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
 bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
 c967f4e2ee0a811e04362aef452b1cfddfd973f3 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 9dae03562aff82574dcda5339df415193655d5a2 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 5726c0e834e6e5ad47f47f3337d9317ee1366955 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
 9115697088cab8fd1732086963c7c3f982069380 


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

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


Testing
---

As there is no functiuonal change I have not added any tests. I made sure that 
existing tests pass.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

> On Feb. 1, 2019, 4:34 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Line 468 (original), 472 (patched)
> > 
> >
> > Log here?

added log to removeAuthzObj.


> On Feb. 1, 2019, 4:34 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Line 481 (original), 487 (patched)
> > 
> >
> > Log here?

added log to removeAuthzObj.


> On Feb. 1, 2019, 4:34 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Line 511 (original), 518 (patched)
> > 
> >
> > Log here?

Only place it is called is deleteIfDangling. We are logging there. These is no 
need to log here again.


- kalyan kumar


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


On Jan. 31, 2019, 4:46 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Jan. 31, 2019, 4:46 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/1/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

> On Feb. 1, 2019, 4:48 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
> > Line 199 (original), 203 (patched)
> > 
> >
> > Why not keep seq num and image num also class name?

This is logged using getSequenceInfo. Which is invoked below.


> On Feb. 1, 2019, 4:48 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
> > Line 209 (original), 213 (patched)
> > 
> >
> > Same as above

This is logged using getSequenceInfo. Which is invoked below.


- kalyan kumar


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


On Jan. 31, 2019, 4:46 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Jan. 31, 2019, 4:46 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/1/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[kalyan kumar kalvagadda via Review Board]

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

Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.


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


Repository: sentry


Description
---

Logging will help us identify below
1. Track the updates from Sentry
2. What the path update is?
3. What the perm update is?
4. How the path change is effecting the exizting cache and what is the result 
of the update?
5. How the perm change is effecting the exizting cache and what is the result 
of the update?
6. Changes to the path tree stored as part of the cache.


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
 bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
 c967f4e2ee0a811e04362aef452b1cfddfd973f3 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 9dae03562aff82574dcda5339df415193655d5a2 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 5726c0e834e6e5ad47f47f3337d9317ee1366955 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
 9115697088cab8fd1732086963c7c3f982069380 


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


Testing
---

As there is no functiuonal change I have not added any tests. I made sure that 
existing tests pass.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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


Fix it, then Ship it!




Please consider the cosmetic comments added. You can address them if you agree 
and commit the change.


sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Line 158 (original), 162 (patched)


Why is thie removed?



sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
Lines 113 (patched)


Don't you think, 
senry.metastore.read.authorization.enabled is better?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Lines 142 (patched)


variable name, readAuthorizationEnabled would be clear,



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Line 158 (original), 163 (patched)


Why is change needed?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Lines 212 (patched)


How about authorizeDatabaseRead?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Lines 217 (patched)


How about authorizeTableRead?


- kalyan kumar kalvagadda


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 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[kalyan kumar kalvagadda via Review Board]

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



Looks good. Wiating for the tests to pass to give a +2.

- kalyan kumar kalvagadda


On Jan. 28, 2019, 6:55 p.m., Na Li wrote:
> 
> ---
> 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
> -
> 
>   
> 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/
> 
> 
> 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

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


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 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[kalyan kumar kalvagadda 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.

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 kumar


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

[kalyan kumar kalvagadda via Review Board]

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



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.

- kalyan kumar kalvagadda


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

[kalyan kumar kalvagadda via Review Board]

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


Fix it, then Ship it!




Wait for the test to complete.


sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
Lines 150 (patched)


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.


- kalyan kumar kalvagadda


On Jan. 25, 2019, 7:57 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, 7:57 p.m.)
> 
> 
> 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 69076: SENTRY-2301: Log where sentry stands in the snapshot fetching process, periodically

[kalyan kumar kalvagadda via Review Board]

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




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


Can you make configurarble? It might be helpfull.
 What do you say?



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


I dont't a value is printing partition-id. Instead we should be looging 
partion name. Yes, it will be big. Information that we are trying to log will 
be any way high regardless.



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


Change "Fetch partition objects for" to "Fetching partitions for ". This 
comment applies to tabletask and dbtask as well.



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


Printing partitionId doesn't tell you anything. Looking at the ID you can 
not derive the parition that has null storage descriptot. You should print 
partition name.



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


This doesn't look correct because results might be still getting appended 
by the tasks.(PartitionTask/TableTask/DbTask)

You should instead print the number of objects for which paths are already 
fetched. Number of objects here include database and tables.

You can use fullSnapshot.size(). This will give the total number of 
databases and tables fetches fetched so far.

This is good enough.


- kalyan kumar kalvagadda


On Jan. 24, 2019, 6:24 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69076/
> ---
> 
> (Updated Jan. 24, 2019, 6:24 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When sentry is fetching snapshot from HMS, it should log periodically on 
> where it stands in the snapshot process. This will help person debugging it 
> and help him understand the progress.
> 
>  
> 
> This is important as this process could take magnitude of minutes when the 
> HMS data is huge.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  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/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69832: SENTRY-2488: Add privilege cache to sentry hive bindings in DefaultAccessValidator

[kalyan kumar kalvagadda via Review Board]

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


Fix it, then Ship it!




Fix it and ship it.


sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
Lines 248-250 (original), 247-256 (patched)


Add comments to new API explaining what it does.



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
Line 831 (original), 831 (patched)


This need not be a public API. Instead you could use no modifier limititng 
it to the package.


- kalyan kumar kalvagadda


On Jan. 24, 2019, 6:23 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69832/
> ---
> 
> (Updated Jan. 24, 2019, 6:23 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: SENTRY-2488
> https://issues.apache.org/jira/browse/SENTRY-2488
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> We are not consistent with behavior in SentryHiveMetaStoreHook (not used 
> anymore) which would cache privileges when authorizing show databases or show 
> tables command. This needs to be added back
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  9de47b338 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  63d8d1c09 
> 
> 
> Diff: https://reviews.apache.org/r/69832/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS ACL synchronization is complete.

[kalyan kumar kalvagadda via Review Board]

> On Sept. 4, 2018, 6:32 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > 
> >
> > This condition should be after getUpdatesFrom methods for path and perm 
> > are called. That is where state SENDING_FULL_UPDATES is set. 
> > 
> > Also this should be an OR for Path Updates or Perm updates. We are 
> > checking for sending full update state as opposed to the inital HDFS ACL 
> > sync
> 
> kalyan kumar kalvagadda wrote:
> This logic is hit when the client sends a request after sentry server 
> sent the full update for perms and paths.
> 
> Arjun Mishra wrote:
> Ok got it. So it is logged on the cosecutive call. I still see a problem 
> though. I think its not fair to say PathUpdate and PermUpdate will be set to 
> SENDING_FULL_UPDATES at the same time. Your logic is assuming that current 
> path and perm update state is SENDING_FULL_UPDATES  but it can be the case 
> that PathUpdate is still at FETCHING_FULL_UPDATES, while PermUpdate state 
> (which usually occurs faster) has completed sending full updates and is now 
> at SENDING_DELTA_UPDATE. That means when PathUpdate completes sending full 
> update, this condition will not be true 
> (SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, 
> PermUpdaterState.SENDING_FULL_UPDATES) && 
> SentryStateBank.isEnabled(PathUpdaterState.COMPONENT, 
> PermUpdaterState.SENDING_FULL_UPDATES))
> 
> Arjun Mishra wrote:
> I think instead of verifying if HDFS ACL sync is complete, we can only 
> log if Path Full Update has been sent, and Perm Full Update has been sent.
> 
> Arjun Mishra wrote:
> The corrent logic should actually be if both Path and Perm update state 
> is at SENDING_DELTA_UPDATES. That is a garuntee that full updates have 
> already been sent, since deltaas are requested only after full updates were 
> sent

Here is the logic makes sure that thar both PathUpdaterState and 
PermUpdaterState are in SENDING_FULL_UPDATES and there was a request from 
Namenode asking for path and perm sequences what which are greater than the 
sequence numbers that were served.

If we change the logic as you suggested it could take longer as there need not 
be updates after a full update was sent.


- kalyan kumar


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


On Jan. 24, 2019, 6:38 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> ---
> 
> (Updated Jan. 24, 2019, 6:38 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently there is no way to confirm that HDFS ACL synchronization is 
> complete when snapshot is initiated. We need to identify that and log in 
> console and log file as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df3ea9126f21248365d6096fcdb83f21e 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
>  81c614a345c122e067ec0a19b8f75766390b2ad4 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  3532ef33dde8b119ab095861381fa52fa5520f4c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
>  8d6713acd61f99940f72c4985098dfeabb9fc832 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
>  443434127b14fadaeb27717ad2370dcdc10ca70c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
>  0cd405b54ee2b4bf788dffb1ac606362614f6efe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
>  2afe919c31b0c5f495d54c6448593abad38eddc2 
> 
> 
> Diff: https://reviews.apache.org/r/68622/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS ACL synchronization is complete.

[kalyan kumar kalvagadda via Review Board]

> On Sept. 5, 2018, 8:27 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 74 (patched)
> > 
> >
> > we should seperate the logic for path update from perm update. They can 
> > be independent.

They need not be because sentry namenode plugin requets for both paths and 
permissions in a single request and sentry responds back with pahs and 
permissions in the same response back.


> On Sept. 5, 2018, 8:27 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
> > Lines 75 (patched)
> > 
> >
> > this line is the same as line 74. Why do you duplicate it?

There was a typo. Good find.


- kalyan kumar


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


On Jan. 24, 2019, 6:38 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> ---
> 
> (Updated Jan. 24, 2019, 6:38 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently there is no way to confirm that HDFS ACL synchronization is 
> complete when snapshot is initiated. We need to identify that and log in 
> console and log file as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df3ea9126f21248365d6096fcdb83f21e 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
>  81c614a345c122e067ec0a19b8f75766390b2ad4 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  3532ef33dde8b119ab095861381fa52fa5520f4c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
>  8d6713acd61f99940f72c4985098dfeabb9fc832 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
>  443434127b14fadaeb27717ad2370dcdc10ca70c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
>  0cd405b54ee2b4bf788dffb1ac606362614f6efe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
>  2afe919c31b0c5f495d54c6448593abad38eddc2 
> 
> 
> Diff: https://reviews.apache.org/r/68622/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS ACL synchronization is complete.

[kalyan kumar kalvagadda via Review Board]

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

(Updated Jan. 24, 2019, 6:38 p.m.)


Review request for sentry and Arjun Mishra.


Changes
---

addressed review comments.


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


Repository: sentry


Description
---

Currently there is no way to confirm that HDFS ACL synchronization is complete 
when snapshot is initiated. We need to identify that and log in console and log 
file as well.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 08b16a4df3ea9126f21248365d6096fcdb83f21e 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
 81c614a345c122e067ec0a19b8f75766390b2ad4 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 3532ef33dde8b119ab095861381fa52fa5520f4c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
 8d6713acd61f99940f72c4985098dfeabb9fc832 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
 443434127b14fadaeb27717ad2370dcdc10ca70c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
 0cd405b54ee2b4bf788dffb1ac606362614f6efe 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
 2afe919c31b0c5f495d54c6448593abad38eddc2 


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

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


Testing
---


Thanks,

kalyan kumar kalvagadda

Re: Review Request 68622: SENTRY-2287: Find a way to report that HDFS ACL synchronization is complete.

[kalyan kumar kalvagadda via Review Board]

- kalyan kumar


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


On Jan. 24, 2019, 6:38 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68622/
> ---
> 
> (Updated Jan. 24, 2019, 6:38 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: SENTRY-2287
> https://issues.apache.org/jira/browse/SENTRY-2287
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently there is no way to confirm that HDFS ACL synchronization is 
> complete when snapshot is initiated. We need to identify that and log in 
> console and log file as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df3ea9126f21248365d6096fcdb83f21e 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
>  81c614a345c122e067ec0a19b8f75766390b2ad4 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  3532ef33dde8b119ab095861381fa52fa5520f4c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
>  8d6713acd61f99940f72c4985098dfeabb9fc832 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
>  443434127b14fadaeb27717ad2370dcdc10ca70c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
>  0cd405b54ee2b4bf788dffb1ac606362614f6efe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PathUpdaterState.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/PermUpdaterState.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
>  2afe919c31b0c5f495d54c6448593abad38eddc2 
> 
> 
> Diff: https://reviews.apache.org/r/68622/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[kalyan kumar kalvagadda via Review Board]

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

  
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/

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


Testing
---

Added new unit tests to test the API added.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69561: SENTRY-2480: Change processDropDatabase to call removeAllPaths

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Looks good. Wait for the tests to complete.

- kalyan kumar kalvagadda


On Dec. 12, 2018, 9:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69561/
> ---
> 
> (Updated Dec. 12, 2018, 9:19 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-2480
> https://issues.apache.org/jira/browse/SENTRY-2480
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now it calls removePaths. Remove paths has a bug that it doesn't check 
> if all associated paths are deleted and the object should be deleted
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  ab262d0a8 
> 
> 
> Diff: https://reviews.apache.org/r/69561/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

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

[kalyan kumar kalvagadda via Review Board]

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



Arjun,


Fix for SENTRY-2249 solves this issue. There is no new fix for this issue.

- kalyan kumar kalvagadda


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

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

[kalyan kumar kalvagadda via Review Board]

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



Arjun,

Fix for SENTRY-2249 solves this issue. There is no new fix for this issue.

- kalyan kumar kalvagadda


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

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Dec. 11, 2018, 12:28 a.m.)


Review request for sentry and Sergio Pena.


Changes
---

Addressed review comments.


Summary (updated)
-

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


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

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

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


Testing
---

Added new unit tests to test the API added.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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




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


I agree with lina's comment.

here is my suggested approach
Currently there is no filter provided while fetching, MAuthzPathsMapping. 
Add a new API which takes the paths as filter so that all the paths are 
fetches. With this, only paths match the pattern are fetches from database.

This solves the issue with fetching all the paths associated with an 
authzObj.


- kalyan kumar kalvagadda


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.

[kalyan kumar kalvagadda via Review Board]

> On Dec. 7, 2018, 7:40 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3932 (patched)
> > 
> >
> > If column name is not null, should add that in filter

I added this API as part og export functionality which did not need column 
privileges. That is why i missed it.
As this API is genneric, it should handle column privileges as well. I will 
address it.


> On Dec. 7, 2018, 7:40 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3943 (patched)
> > 
> >
> > 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

I will address it.


- kalyan kumar


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


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

[kalyan kumar kalvagadda via Review Board]

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

  
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/

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


Testing
---

Made sure that existiung tests pass.


Thanks,

kalyan kumar kalvagadda

Review Request 69517: SENTRY-2475: Add the list of authorizables to the export file

[kalyan kumar kalvagadda via Review Board]

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

Review request for sentry and Sergio Pena.


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


Repository: sentry


Description
---

With the new changes proposed, sentry should be able to accept a list of 
authorizables for which permissions should be exported. In this process, sentry 
should update the the list authorizables to the export file along with the 
permissions/role/groups etc.

This information can be used by sentry to cleanup the existing permissions 
before starting importing the data.


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
 b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
 7baeee17339d29576b841480e600a3cfaf14adf3 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
 216d8612bee688057aa834b1a96045bb7df6b9c4 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 68d864cfbdf18057d87a65a04af8991292aadccf 


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


Testing
---

Updated tests to verify this new functionality 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.

[kalyan kumar kalvagadda via Review Board]

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

  
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/

Changes: https://reviews.apache.org/r/69353/diff/1-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.

[kalyan kumar kalvagadda via Review Board]

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

Here is the basic idea. This is the suedo code to understand how the new api is 
used.

  public Map> 
getRoleNameTPrivilegesMap(List authorizables) throws 
Exception {
return tm.executeTransaction(
pm -> {
  List mSentryPrivileges = 
**getPrivilegesForAuthorizables(pm, authorizables);**
  return getRolePrivilegesMap(mSentryPrivileges);
}); 
  }


- kalyan kumar


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


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

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Dec. 6, 2018, 9:19 p.m.)


Review request for sentry and Sergio Pena.


Changes
---

Addressed review comments.


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


Repository: sentry


Description
---

Sentry should be able to export permission information to a HDFS location.

This can be done using hadoop-common library.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
 b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryPolicyFileFormatter.java
 4f465b3671c1fdd6de7cd0773a29108af40311c8 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java
 5f1e3e916d71c51e19d3d37ba788f902ed6b7e27 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
 7baeee17339d29576b841480e600a3cfaf14adf3 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 092060c450c6a906850630cb10454737157af5fe 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
 cf1fdab382034c8950da2613ef9b0cf1af912e33 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
 df9299c5df26c891ceebbc59b79dd0f7cd3ceeb4 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 b048989b42bde0318c6d0fa0e9353a2a59954407 


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

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


Testing
---

Added new tests to verify the new behavior added.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Dec. 6, 2018, 8:23 p.m.)


Review request for sentry and Sergio Pena.


Changes
---

Addressed reivew comments.


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

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
 d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
 6eb85217b9229438b9adbd1546a408dc507f1645 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java
 13e57e04dd779825e2986b1e527f4e556271a162 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java
 21f3bdf5e304fe2f49684c999d0aa0e0362320de 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java
 cea868f0524c25bf63cfb7c532829354a280df28 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 83393a98df5e10b324f74dc12f10c20bc5f02651 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 68d864cfbdf18057d87a65a04af8991292aadccf 
  
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 3364648dc88acbb8de1cc925fe8c512f34a4b064 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
 2dc0975f482f760c18c438c1418630a7ef6a4cbb 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 b9e3bf2921a0696da639e1b1bee5d83cf2b9cee0 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
 cf1fdab382034c8950da2613ef9b0cf1af912e33 


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

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


Testing
---

Made sure that existiung tests pass.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Dec. 5, 2018, 11:23 p.m.)


Review request for sentry and Sergio Pena.


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


Repository: sentry


Description
---

TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are 
used to send export and import requests to sentry server should be changed to 
be able to send a list authorizables for which permission information has to be 
exported/imported.

These requests should accommodate source and target cluster information as well.


Diffs (updated)
-

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

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


Testing
---

Made sure that existiung tests pass.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Dec. 5, 2018, 10:19 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 (updated)
-

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

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


Testing
---

Made sure that existiung tests pass.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Dec. 5, 2018, 10:18 p.m.)


Review request for sentry and Sergio Pena.


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


Repository: sentry


Description
---

Sentry should be able to export permission information to a HDFS location.

This can be done using hadoop-common library.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
 b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryPolicyFileFormatter.java
 4f465b3671c1fdd6de7cd0773a29108af40311c8 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java
 5f1e3e916d71c51e19d3d37ba788f902ed6b7e27 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
 7baeee17339d29576b841480e600a3cfaf14adf3 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 092060c450c6a906850630cb10454737157af5fe 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
 cf1fdab382034c8950da2613ef9b0cf1af912e33 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
 df9299c5df26c891ceebbc59b79dd0f7cd3ceeb4 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 b048989b42bde0318c6d0fa0e9353a2a59954407 


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

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


Testing
---

Added new tests to verify the new behavior added.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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



I understand there is a gap here but we should be carefull in changing the 
current behavior.I assume, the idea here is to find a way to remove all the 
privileges granted to an authrozable(server/database/table)

Using "ALL" to soleve is not backward compatable. There could be customers who 
want to remove only "ALL" privilege and nothing else when "ALL" is revoked. 
Instead we should try to solve this with out using "ALL".

- kalyan kumar kalvagadda


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

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

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


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.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/TestSentryWebServerWithoutSecurity.java
>  6e741e895 
>   sentry-service/sentry-service-web/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-web/src/main/java/org/apache/sentry/service/web/DefaultWebServicesProvider.java
>  PRE-CREATION 

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

[kalyan kumar kalvagadda via Review Board]

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

  
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/

Changes: https://reviews.apache.org/r/69087/diff/10-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.

[kalyan kumar kalvagadda 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)
> > 
> >
> > do you want to clear pathsToPersist after persisting them? Otherwise, 
> > next time it is called, those paths will be added again

Persisting is the last thing that we do on an instance of MAuthzPathsMapping. 
This shouldn't be an issue.


- kalyan kumar


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


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.

[kalyan kumar kalvagadda via Review Board]

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

  
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/

Changes: https://reviews.apache.org/r/69087/diff/9-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.

[kalyan kumar kalvagadda via Review Board]

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

(Updated Nov. 29, 2018, 6:29 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

addresed comment.


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

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

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


Testing
---

Made sure all the unit tests passed.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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

Review request for sentry and Sergio Pena.


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


Repository: sentry


Description
---

Sentry should be able to export permission information to a HDFS location.

This can be done using hadoop-common library.


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
 b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryPolicyFileFormatter.java
 4f465b3671c1fdd6de7cd0773a29108af40311c8 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java
 5f1e3e916d71c51e19d3d37ba788f902ed6b7e27 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
 7baeee17339d29576b841480e600a3cfaf14adf3 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
 cf1fdab382034c8950da2613ef9b0cf1af912e33 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
 df9299c5df26c891ceebbc59b79dd0f7cd3ceeb4 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 b048989b42bde0318c6d0fa0e9353a2a59954407 


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


Testing
---

Added new tests to verify the new behavior added.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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




sentry-service/sentry-service-web/src/main/webapp/static/bootstrap/js/bootstrap-3.3.7.js
Lines 1 (patched)


Brian, 

What does this bootstrap java script do?


- kalyan kumar kalvagadda


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/TestSentryWebServerWithoutSecurity.java
>  6e741e895 
>   sentry-service/sentry-service-web/pom.xml 

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

[kalyan kumar kalvagadda via Review Board]

> On Nov. 20, 2018, 5:20 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Line 36 (original), 45 (patched)
> > 
> >
> > Maintain both mPaths and paths in an instance of MAuthzPathsMapping is 
> > easy to have bug.
> > 
> > Can you only have private Set mPaths and not have paths?
> > 
> > Then inside of this.makePersistent(), you create MPathToPersist from 
> > MPath value. 
> > 
> > That will minimize the changes to make, and avoid some scenarios you 
> > did not cover when having both mPaths and paths.

Doing that is not efficient because we will be converting strings -> MPath -> 
MPathToPesist. That is why i took this route. Let me think.
More over using collection to hold that should be persisted would make 
the code much complex. I have already tried it. It will lead to bugs in the 
future


> On Nov. 20, 2018, 5:20 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Line 89 (original), 99 (patched)
> > 
> >
> > since you have both mPaths and paths, if the call called addPath then 
> > getPath right away, the result is not correct. It is much easier to have 
> > just mPaths.

It may look easier to have just mPaths but it is not. I have spent hours to 
make it work. It was becoming more complex.


- kalyan kumar


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


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

[kalyan kumar kalvagadda via Review Board]

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


Changes
---

Addressed reiview comments from lina.


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

  
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/

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


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.

[kalyan kumar kalvagadda via Review Board]

> On Nov. 1, 2018, 1:35 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Lines 37 (patched)
> > 
> >
> > This is already defined in the package.jdo. Does it need to be defined 
> > here as well? My understanding is that it can be either on the package.jdo 
> > or the class file or both, but should we be consistent where we put these 
> > declarations?

That may be true as per documentation but I followed the convension used in 
sentry currently. We do mention it in both places. It should be of no harm.


> On Nov. 1, 2018, 1:35 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Line 36 (original), 42 (patched)
> > 
> >
> > Is this variable needed now that you remove the getters and setters?
> > 
> > Btw, why isn't this variable used anymore?

Collection of MPaths is initialized when snapshot is retrieved from database 
and is used with getPathStrings API.


> On Nov. 1, 2018, 1:35 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
> > Lines 259-261 (original), 259-261 (patched)
> > 
> >
> > What is the strategy for the ID? Will it continue using the increment 
> > strategy?

It is not auto incremented. It is incremented in the application.


> On Nov. 1, 2018, 1:35 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
> > Lines 300 (patched)
> > 
> >
> > Why a new class that points to the same table as MPath is needed?

This new class is added to make sure sure that batch insert can happen. With 
MPath, that can not be performed?


- kalyan kumar


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


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

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

[kalyan kumar kalvagadda via Review Board]

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

Review request for sentry and Sergio Pena.


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


Repository: sentry


Description
---

New sentry API should be implemented to fetch the privileges granted to 
authorizables and it's children. authorizables include database, tables, 
columns and URI's.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 e48eea377b842475f72b6fab4567a82c8fd93098 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 


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


Testing
---

Added new unit tests to test the API added.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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

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


Testing
---

Made sure that existiung tests pass.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69260: SENTRY-2441: When MAuthzPathsMapping is deleted all associated MPaths should be deleted automatically.

[kalyan kumar kalvagadda via Review Board]

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

(Updated Nov. 6, 2018, 6:08 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

Added unit test case


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


Repository: sentry


Description
---

When MAuthzPathsMapping is deleted all associated MPaths should be deleted 
automatically as MPaths are directly associated to MAuthzPathsMapping.

 

Currently when entries in MAuthzPathsMapping are deleted the FK in MPaths is 
set to NULL and the entries are left. These elements are slate are not not used 
and will never be cleaned.

 

 

Instead MPaths should be removed automatically associated MAuthzPathsMapping 
entries are deleted.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 20ec0deab6b97065cfe99beea3d14a6c7268aac3 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 33c40613a05f7c7fde314af6aba6b269bf6ffaae 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 66db6ae9a436b9728fb3c2ebdd21167ef042f937 


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

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


Testing
---

Made sure that all the existing tests pass.


Thanks,

kalyan kumar kalvagadda

Review Request 69260: SENTRY-2441: When MAuthzPathsMapping is deleted all associated MPaths should be deleted automatically.

[kalyan kumar kalvagadda via Review Board]

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

Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


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


Repository: sentry


Description
---

When MAuthzPathsMapping is deleted all associated MPaths should be deleted 
automatically as MPaths are directly associated to MAuthzPathsMapping.

 

Currently when entries in MAuthzPathsMapping are deleted the FK in MPaths is 
set to NULL and the entries are left. These elements are slate are not not used 
and will never be cleaned.

 

 

Instead MPaths should be removed automatically associated MAuthzPathsMapping 
entries are deleted.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 20ec0deab6b97065cfe99beea3d14a6c7268aac3 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 33c40613a05f7c7fde314af6aba6b269bf6ffaae 


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


Testing
---

Made sure that all the existing tests pass.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Nov. 2, 2018, 6:40 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69212/
> ---
> 
> (Updated Nov. 2, 2018, 6:40 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2329
> https://issues.apache.org/jira/browse/sentry-2329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump hadoop.version to 3.1.1. The changes on Sentry are compatible with 
> Hadoop 2.7.
> 
> 
> Diffs
> -
> 
>   pom.xml acbdcc2722bf189811cb528ac1b2d07983a571c2 
>   sentry-binding/sentry-binding-solr/pom.xml 
> f2a5fca76d3d220fcf2b72a3179ff5218fc6577c 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> df6f04c048b502ff5f8e8ec397d75166faba8c3c 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  c9ecc4021b167d98c7dade409c97ae7d26e967ea 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  18b62652a11dfee6683cb8f24944ccd3d344dc9f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithKerberos.java
>  5d94d4bc6a2a47189e69556a5e4d9bdee05952a7 
> 
> 
> Diff: https://reviews.apache.org/r/69212/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

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

[kalyan kumar kalvagadda via Review Board]

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



Sergio,

I remeber we taking about supporting multiple versions of Hadoop somehow but 
this change is not backward compatible. People using sentry 2.2 are forced to 
use Hadoop 3.1.1 only. Is that correct?

- kalyan kumar kalvagadda


On Nov. 2, 2018, 6:40 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69212/
> ---
> 
> (Updated Nov. 2, 2018, 6:40 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2329
> https://issues.apache.org/jira/browse/sentry-2329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump hadoop.version to 3.1.1. The changes on Sentry are compatible with 
> Hadoop 2.7.
> 
> 
> Diffs
> -
> 
>   pom.xml acbdcc2722bf189811cb528ac1b2d07983a571c2 
>   sentry-binding/sentry-binding-solr/pom.xml 
> f2a5fca76d3d220fcf2b72a3179ff5218fc6577c 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> df6f04c048b502ff5f8e8ec397d75166faba8c3c 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  c9ecc4021b167d98c7dade409c97ae7d26e967ea 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  18b62652a11dfee6683cb8f24944ccd3d344dc9f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithKerberos.java
>  5d94d4bc6a2a47189e69556a5e4d9bdee05952a7 
> 
> 
> Diff: https://reviews.apache.org/r/69212/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

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

[kalyan kumar kalvagadda via Review Board]

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




pom.xml
Lines 216-231 (patched)


I think hadoop-minicluster can be made a test only dependency as it is used 
only for testing by changing the scope to test.

If we are good with this we don't have to exclude these as these 
dependencies are not propogated anyway.



sentry-binding/sentry-binding-solr/pom.xml
Lines 32-47 (patched)


Can you elaborate on this?

If the scope of hadoop-minicluster is changed to test, would there stil be 
issue with the jetty version?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
Lines 334-338 (patched)


What made you add this check?

Was there something that was failing OR Just added for safety?


- kalyan kumar kalvagadda


On Oct. 30, 2018, 2:16 a.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69212/
> ---
> 
> (Updated Oct. 30, 2018, 2:16 a.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2329
> https://issues.apache.org/jira/browse/sentry-2329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump hadoop.version to 3.1.1. The changes on Sentry are compatible with 
> Hadoop 2.7.
> 
> 
> Diffs
> -
> 
>   pom.xml acbdcc2722bf189811cb528ac1b2d07983a571c2 
>   sentry-binding/sentry-binding-solr/pom.xml 
> f2a5fca76d3d220fcf2b72a3179ff5218fc6577c 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> df6f04c048b502ff5f8e8ec397d75166faba8c3c 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  c9ecc4021b167d98c7dade409c97ae7d26e967ea 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  18b62652a11dfee6683cb8f24944ccd3d344dc9f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithKerberos.java
>  5d94d4bc6a2a47189e69556a5e4d9bdee05952a7 
> 
> 
> Diff: https://reviews.apache.org/r/69212/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Oct. 31, 2018, 11:28 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

updated new patch


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

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
 d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 092060c450c6a906850630cb10454737157af5fe 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
 c51f25a0393b482814afcd3b7a19e547b689ac6e 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
 b0eaff2120a80685da07c65a7706edf2be62ee01 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPathToPersist.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 20ec0deab6b97065cfe99beea3d14a6c7268aac3 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 33c40613a05f7c7fde314af6aba6b269bf6ffaae 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 66db6ae9a436b9728fb3c2ebdd21167ef042f937 


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

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


Testing
---

Made sure all the unit tests passed.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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



Xinran,

What is the purpose of such annotation?

- kalyan kumar kalvagadda


On Oct. 29, 2018, 4:45 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69201/
> ---
> 
> (Updated Oct. 29, 2018, 4:45 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry bindings are using some of the classes in sentry.core and common. 
> These classes are annotated as public
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ActiveRoleSet.java
>  c24a6cde 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  88457c0d 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Column.java
>  e36b09a1 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAction.java
>  c5842d98 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java
>  4ce01b2c 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Database.java
>  e8dc1406 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Server.java
>  41693c25 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Table.java
>  5a981588 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java
>  aecfe5b5 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
>  761fb527 
> 
> 
> Diff: https://reviews.apache.org/r/69201/diff/1/
> 
> 
> Testing
> ---
> 
> mvn clean install
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

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

[kalyan kumar kalvagadda via Review Board]

> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
> > Lines 153 (patched)
> > 
> >
> > is there an optimal value higher than 2 to make it faster?
> 
> kalyan kumar kalvagadda wrote:
> I used 2 because it is minimum value. We can increase the number but the 
> optimal number depends on the hardware used and the cpu usage.

any smaller number(2-3) should be good. It's not good idea to have a bigger 
value as we don't have a control on kind of hardware where is sentry is run.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3476-3479 (original), 3491-3494 (patched)
> > 
> >
> > I wonder how much memory do we expect in the worst case scenarios to be 
> > hold in memory for all these objects in the pool?
> > 
> > In the previous patchthe objects were persisted (1 at a time), and GC 
> > could collected them.
> 
> kalyan kumar kalvagadda wrote:
> That is the case even now GC should collect them. Only difference is that 
> there is additional memory usage for short period of time. It is not possible 
> to come up with exact memory usage. In the worst case it could be size of the 
> fullsnapshot.
> I can make code changes to limit the queue length to avaoid additional 
> memory usage.

This appraach reduces memory usage signifiacantly as we are not holding up the 
data in L-1 cache.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3478 (original), 3493 (patched)
> > 
> >
> > This is executed in a different transaction?
> 
> Arjun Mishra wrote:
> I believe its all in a single transaction
> 
> kalyan kumar kalvagadda wrote:
> it is not possible to perform concurrent inserts using a single 
> transaction. SnapshotUpdater is executed as seperate transaction. In the 
> event if failure we need to perform explicit cleanup.

Yes, This is different transaction.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3510 (patched)
> > 
> >
> > Aren't these deleted during the rollback?
> 
> Arjun Mishra wrote:
> Does executeTransactionWithRetry include roll back on failure?
> 
> kalyan kumar kalvagadda wrote:
> it is not possible to perform concurrent inserts using a single 
> transaction. SnapshotUpdater is executed as seperate transaction. In the 
> event if failure we need to perform explicit cleanup.

As we are persisinting the paths for a hive table/database is seperate 
transaction we need to have an explcitiy logic to remove the other paths in the 
event of failure.


- kalyan kumar


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


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

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Oct. 31, 2018, 12:58 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

Fixed some issue observed in mu testing.


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.

This patch will change with SENTRY-2305. This review request is just to give 
you a sence of the change.


Diffs (updated)
-

  
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/MPathToPersist.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 20ec0deab6b97065cfe99beea3d14a6c7268aac3 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 33c40613a05f7c7fde314af6aba6b269bf6ffaae 


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

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


Testing
---


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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


Fix it, then Ship it!




With this change you are able to levarage the batching capability of 
datanucelus which makes it faster. 
Please fix them and ship it.


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


Could you please add a comment for this method explaning it. It will make 
code readable. It was missing before. It's good to add them when we get a 
chance.



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


Could you please add a comment for this method explaning it. It will make 
code readable. It was missing before. It's good to add them when we get a 
chance.


- kalyan kumar kalvagadda


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

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

[kalyan kumar kalvagadda via Review Board]

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


Fix it, then Ship it!




Fix it and ship it.


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


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

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


- kalyan kumar kalvagadda


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

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Oct. 24, 2018, 4:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 24, 2018, 4:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/2/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[kalyan kumar kalvagadda via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
Lines 375-379 (patched)


It can be simply changed to 

addCustomParam(prefix + ":" + vName, vName, (toLowerCase) ? 
name.trim().toLowerCase() : name.trim());



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


you can just call princilaNames.safeTrim()


- kalyan kumar kalvagadda


On Oct. 22, 2018, 10:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 22, 2018, 10:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/1/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

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

[kalyan kumar kalvagadda via Review Board]

> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
> > Lines 152 (patched)
> > 
> >
> > how can we make this variable name to not be confused with threads 
> > fetching an hms snapshot?

I don't think the name is confusing.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
> > Lines 153 (patched)
> > 
> >
> > is there an optimal value higher than 2 to make it faster?

I used 2 because it is minimum value. We can increase the number but the 
optimal number depends on the hardware used and the cpu usage.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3476-3479 (original), 3491-3494 (patched)
> > 
> >
> > I wonder how much memory do we expect in the worst case scenarios to be 
> > hold in memory for all these objects in the pool?
> > 
> > In the previous patchthe objects were persisted (1 at a time), and GC 
> > could collected them.

That is the case even now GC should collect them. Only difference is that there 
is additional memory usage for short period of time. It is not possible to come 
up with exact memory usage. In the worst case it could be size of the 
fullsnapshot.
I can make code changes to limit the queue length to avaoid additional memory 
usage.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3478 (original), 3493 (patched)
> > 
> >
> > This is executed in a different transaction?
> 
> Arjun Mishra wrote:
> I believe its all in a single transaction

it is not possible to perform concurrent inserts using a single transaction. 
SnapshotUpdater is executed as seperate transaction. In the event if failure we 
need to perform explicit cleanup.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3481 (original), 3496 (patched)
> > 
> >
> > Shouldn't we destroy or stop the persisting process of the rest of the 
> > paths if the persistingSnapshotFailed is true?
> 
> Arjun Mishra wrote:
> Yeah we do that. persistingSnapshotFailed is set to true when a single 
> processing fails with exception

we already do that using flag persistingSnapshotFailed.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3510 (patched)
> > 
> >
> > Aren't these deleted during the rollback?
> 
> Arjun Mishra wrote:
> Does executeTransactionWithRetry include roll back on failure?

it is not possible to perform concurrent inserts using a single transaction. 
SnapshotUpdater is executed as seperate transaction. In the event if failure we 
need to perform explicit cleanup.


> On Oct. 22, 2018, 4:18 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 5106-5112 (patched)
> > 
> >
> > How often does this code get ID conflicts while persisting in a 
> > threaded process? Isn't this going to make the persisting slow?

As the PATH_ID is auto generated there will not be any conflicts there may be 
some gaps.


- kalyan kumar


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


On Oct. 22, 2018, 2:42 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68973/
> ---
> 
> (Updated Oct. 22, 2018, 2:42 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2305
> https://issues.apache.org/jira/browse/SENTRY-2305
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have considered multiple options. 

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Oct. 22, 2018, 2:42 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

addressed review comments.


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


Repository: sentry


Description
---

I have considered multiple options. Persisting in batches is not an option with 
out changing the schema as the data nucleus does not persist row in batches for 
tables which have foreign key on other tables.

I see that best option is to persist the paths in parallel. It gave good 
results.

Solution Approach:

I have used a thread pool to persist the snapshot. Size of this thread pool is 
configurable. Paths for each object database/table are submitted to this thread 
pool. If for reason some of the paths are not pesisted, snapshot is removed and 
exception is throw back.

  This patch along with SENTRY-2423 was 5 times faster when tested with below.

 

Object Type Count
Databases   209
Tables  2100
Partitions  24


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 092060c450c6a906850630cb10454737157af5fe 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22e40b8958395e1c12af12272b89a778726 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 4a9afe303672baff39be01d4f190034b2bfb75fe 


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

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


Testing
---

Added new test and also made sure that existing tests passed.


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

> On Oct. 19, 2018, 7:12 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 187 (patched)
> > 
> >
> > should this variable be local to persistFullPathsImage, and be atomic 
> > as it is set by more than one thread.

If it is local variable it would be not avaiable in SnapshotUpdater. member 
variables of SentryStore will be available to instances of SnapshotUpdater as 
it is a inner class for sentrystore.


> On Oct. 19, 2018, 7:12 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 294 (patched)
> > 
> >
> > When full snapshot fetch is done, should these counters be reset? 
> > Otherwise, they will keep on increasing, more than 
> > totalNumberOfObjectsToPersist

You are right. I will fix it. So yo have any other comments on this patch?


- kalyan kumar


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


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

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

[kalyan kumar kalvagadda via Review Board]

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

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.

This patch will change with SENTRY-2305. This review request is just to give 
you a sence of the change.


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/MPathToPersist.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 20ec0deab6b97065cfe99beea3d14a6c7268aac3 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22e40b8958395e1c12af12272b89a778726 


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


Testing
---


Thanks,

kalyan kumar kalvagadda

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

[kalyan kumar kalvagadda via Review Board]

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

(Updated Oct. 19, 2018, 4:53 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

Addressed review comments.


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


Repository: sentry


Description
---

I have considered multiple options. Persisting in batches is not an option with 
out changing the schema as the data nucleus does not persist row in batches for 
tables which have foreign key on other tables.

I see that best option is to persist the paths in parallel. It gave good 
results.

Solution Approach:

I have used a thread pool to persist the snapshot. Size of this thread pool is 
configurable. Paths for each object database/table are submitted to this thread 
pool. If for reason some of the paths are not pesisted, snapshot is removed and 
exception is throw back.

  This patch along with SENTRY-2423 was 5 times faster when tested with below.

 

Object Type Count
Databases   209
Tables  2100
Partitions  24


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 092060c450c6a906850630cb10454737157af5fe 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22e40b8958395e1c12af12272b89a778726 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 4a9afe303672baff39be01d4f190034b2bfb75fe 


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

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


Testing
---

Added new test and also made sure that existing tests passed.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[kalyan kumar kalvagadda via Review Board]

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

(Updated Oct. 18, 2018, 5:38 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

addressed review comments.


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


Repository: sentry


Description
---

New metric should be added to track the time taken by sentry server in 
granting/updating owner privileges.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 214d78c5332d4113e440b76077bb7b2a6dae81d6 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c388689b63d5db7d71cd6cc47952d647bf 


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

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


Testing
---


Thanks,

kalyan kumar kalvagadda

Re: Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[kalyan kumar kalvagadda via Review Board]

> On Oct. 9, 2018, 6:19 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
> > Lines 124 (patched)
> > 
> >
> > Can we have one for granting and one for revoking owner privilege?
> 
> kalyan kumar kalvagadda wrote:
> Do you see a value in having seperate merics?
> 
> Arjun Mishra wrote:
> Is there a plan in the future to allow explicit granting and revoking of 
> owner privileges? If so, its better to keep them seperated

There is no plan to provide a way to explcitly revoke owner privilege. I will 
be adding metrics for adding and updating owner privileges.


- kalyan kumar


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


On Oct. 9, 2018, 6:01 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68968/
> ---
> 
> (Updated Oct. 9, 2018, 6:01 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2425
> https://issues.apache.org/jira/browse/SENTRY-2425
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New metric should be added to track the time taken by sentry server in 
> granting/updating owner privileges.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6a41ba01943bd3b2b42c9bddc9fa722f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c388689b63d5db7d71cd6cc47952d647bf 
> 
> 
> Diff: https://reviews.apache.org/r/68968/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>