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

2019-02-20 Thread Na Li via Review Board 

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

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


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


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


Repository: sentry


Description
---

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

To improve performance, we need to add cache in SentryMetaStoreFilterHook


Diffs (updated)
-

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


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

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


Testing
---

existing HMS tests succeeded


Thanks,

Na Li

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

2019-02-20 Thread 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

2019-02-20 Thread Na Li via Review Board 


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

what is the abstract class would you suggest?

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

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


- Na


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


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

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

2019-02-20 Thread 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
> 
>

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

2019-02-19 Thread Na Li via Review Board 

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

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


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


Repository: sentry


Description
---

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

To improve performance, we need to add cache in SentryMetaStoreFilterHook


Diffs
-

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


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


Testing
---

existing HMS tests succeeded


Thanks,

Na Li