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

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 9:18 p.m., kalyan kumar kalvagadda wrote:
> > 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, as discussed that approaach won't work as AUTHZ_PATHS_MAPPING object 
retrieved will get all references to all associated AUTHZ_PATH objects


- Arjun


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


On Dec. 11, 2018, 12:07 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 11, 2018, 12:07 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-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/4/
> 
> 
> 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

[Arjun Mishra via Review Board]

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

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


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


Changes
---

Post Lina feedback


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

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

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


Testing
---

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


Thanks,

Arjun Mishra

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

[Na Li via Review Board]

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




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


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


- Na Li


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

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

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


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

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

[Na Li via Review Board]

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



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

- Na Li


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

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

[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 69530: SENTRY-2476: Optimize deleting specific paths for objects

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 4:57 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3455 (original), 3452 (patched)
> > 
> >
> > if it possible that there are multiple instances in table AUTHZ_PATH, 
> > they are of the same value of "PATH_NAME", but belong to different  
> > "authzObj"?
> > 
> > If that is the case, you delect more entries than you need to. 
> > Basically, you delete all entries matching input "paths" regardless the 
> > value of "authzObj". That is not right.

That would be the case with current scenario too. If you had 2 objects "tbl1", 
and "tbl2" mapped to a single path "/user/hive/warehouse/tbl", if we delete 
tbl1, we also delete the path "/user/hive/warehouse/tbl".

Let me run a test case for it


- Arjun


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


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 69530: SENTRY-2476: Optimize deleting specific paths for objects

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 4:57 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3454 (original), 3451 (patched)
> > 
> >
> > Why do you remove the input of "currentSnapshotID"? There could be 
> > multiple snapshots, and we need to make sure to remove the one in the 
> > latest Snapsho

WE don't need to check "currentSnapshotID". Right now there is no way to 
retrieve paths for a specific snapshot Id without getting it from authz mapping 
objects.


- Arjun


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


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

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

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 4:57 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3551 (original), 3539 (patched)
> > 
> >
> > why do you remove input "currentSnapshotID"

We don't need to check "currentSnapshotID". When deleting paths we can delete 
paths irrespective of the snapshot Id they belong to. Right now there is no way 
to retrieve paths for a specific snapshot Id without getting it from authz 
mapping objects.


- Arjun


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


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

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

[Na Li via Review Board]

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



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

- Na Li


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

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

[Na Li via Review Board]

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




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


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



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


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

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



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


why do you remove input "currentSnapshotID"



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


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


- Na Li


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