Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2019-02-06 Thread Haley Reeve via Review Board 

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


Ship it!




Ship It!

- Haley Reeve


On Feb. 6, 2019, 7:35 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65866/
> ---
> 
> (Updated Feb. 6, 2019, 7:35 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, Steve 
> Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2146
> https://issues.apache.org/jira/browse/SENTRY-2146
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There are a bunch of improvements that should be made to 
> ResourceAuthorizationProvider. For example, exceptions thrown by 
> privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
> hard. 
> 
> We also need to add a lot more logging that needs to be added to related 
> classes
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  7565a34b5 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  09bd9b566 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  447deaf58 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  4e944e5f2 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  ab5560994 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f319 
> 
> 
> Diff: https://reviews.apache.org/r/65866/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-binding/sentry-binding-hive/pom.xml test
> mvn -f sentry-core/sentry-core-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-engine/pom.xml test
> mvn -f sentry-provider/sentry-provider-common/pom.xml test
> 
> 
> File Attachments
> 
> 
> New diff. Not able to update
>   
> https://reviews.apache.org/media/uploaded/files/2019/02/06/5b613338-7181-4bbe-bac5-66129cdbc095__SENTRY-2146.04.patch
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2019-02-06 Thread Arjun Mishra via Review Board 

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

(Updated Feb. 6, 2019, 7:35 p.m.)


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


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


Repository: sentry


Description
---

There are a bunch of improvements that should be made to 
ResourceAuthorizationProvider. For example, exceptions thrown by 
privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
hard. 

We also need to add a lot more logging that needs to be added to related classes


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 7565a34b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b566 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf58 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
 4e944e5f2 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
 ab5560994 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 a9b98f319 


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


Testing
---

mvn -f sentry-binding/sentry-binding-hive/pom.xml test
mvn -f sentry-core/sentry-core-common/pom.xml test
mvn -f sentry-policy/sentry-policy-common/pom.xml test
mvn -f sentry-policy/sentry-policy-engine/pom.xml test
mvn -f sentry-provider/sentry-provider-common/pom.xml test


File Attachments (updated)


New diff. Not able to update
  
https://reviews.apache.org/media/uploaded/files/2019/02/06/5b613338-7181-4bbe-bac5-66129cdbc095__SENTRY-2146.04.patch


Thanks,

Arjun Mishra

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-07-27 Thread Arjun Mishra via Review Board 

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

(Updated July 27, 2018, 7:55 p.m.)


Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and 
Sergio Pena.


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


Repository: sentry


Description
---

There are a bunch of improvements that should be made to 
ResourceAuthorizationProvider. For example, exceptions thrown by 
privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
hard. 

We also need to add a lot more logging that needs to be added to related classes


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 7565a34b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b566 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf58 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
 4e944e5f2 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
 ab5560994 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 a9b98f319 


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


Testing
---

mvn -f sentry-binding/sentry-binding-hive/pom.xml test
mvn -f sentry-core/sentry-core-common/pom.xml test
mvn -f sentry-policy/sentry-policy-common/pom.xml test
mvn -f sentry-policy/sentry-policy-engine/pom.xml test
mvn -f sentry-provider/sentry-provider-common/pom.xml test


Thanks,

Arjun Mishra

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-06-06 Thread kalyan kumar kalvagadda via Review Board 

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



I understand that we are trying to improve logging but adding changes to 
specific class might not cover things at breadth.

 

Instead, it would be better to scenario's. Here are some of them

Granting Privilege
Revoke Privilege
Authorization Request
HMS Fullsnapshot
HDFS Full Update
HMS notification fetch/processing
Delta Updates to NN plug-in

- kalyan kumar kalvagadda


On June 5, 2018, 9:01 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65866/
> ---
> 
> (Updated June 5, 2018, 9:01 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There are a bunch of improvements that should be made to 
> ResourceAuthorizationProvider. For example, exceptions thrown by 
> privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
> hard. 
> 
> We also need to add a lot more logging that needs to be added to related 
> classes
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  7565a34b5 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  09bd9b566 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  447deaf58 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  4e944e5f2 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  ab5560994 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f319 
> 
> 
> Diff: https://reviews.apache.org/r/65866/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-binding/sentry-binding-hive/pom.xml test
> mvn -f sentry-core/sentry-core-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-engine/pom.xml test
> mvn -f sentry-provider/sentry-provider-common/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-16 Thread Arjun Mishra via Review Board 

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

(Updated March 16, 2018, 11:01 a.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and 
Xinran Tinney.


Changes
---

Post feedback from Steve and Sergio


Repository: sentry


Description
---

There are a bunch of improvements that should be made to 
ResourceAuthorizationProvider. For example, exceptions thrown by 
privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
hard. 

We also need to add a lot more logging that needs to be added to related classes


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 7565a34b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b566 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf58 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
 4e944e5f2 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
 ab5560994 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 a9b98f319 


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

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


Testing
---

mvn -f sentry-binding/sentry-binding-hive/pom.xml test
mvn -f sentry-core/sentry-core-common/pom.xml test
mvn -f sentry-policy/sentry-policy-common/pom.xml test
mvn -f sentry-policy/sentry-policy-engine/pom.xml test
mvn -f sentry-provider/sentry-provider-common/pom.xml test


Thanks,

Arjun Mishra

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-12 Thread Sergio Pena via Review Board 

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




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


Why was it moved? Didn't make sense to have input and output privileges 
logged before checking for authorization?



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


is the LOG.isDebugEnabled() necessary? The LOG.debug() arguments are not 
doing any intensitve to have this validation, aren't they?

And, what would 'this' print in the log?

Should'nt this log being at the beginning of the method?



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


Same question about the LOG.isDebugEnabled().



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


Why was it removed? isn't it needed anymore?



sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
Line 39 (original), 42 (patched)


Why are these unchecked exceptions part of the constructor declaration?

I think that only checked exceptions are useful to have here. Unchecked 
exceptions are errors that are thrown that are not expected to happen, aren't 
they? Like a NullPointerException.



sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
Lines 48 (patched)


Do we need this validation? privilegeStr is a simple string, so I don't 
think it causes harm if we leave the LOG.debug() call without checking if debug 
is enabled.



sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
Line 23 (original), 23 (patched)


Same question about unchecked exceptions.



sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
Line 26 (original), 26 (patched)


Same question about unchecked exceptions.



sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Lines 112-115 (original), 111 (patched)


Why is this code removed? Isn't this patch only improving log messages?



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


Same question about isDebugEnabled.



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


Same question about isDebugEnabled.



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


Are we really getting privileges from cache?



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


How useful are these error messages for a user? What argument is illegal? 
Is there a way to handle these errors differently than just rethrowing the 
erros and logging the messages?



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


How useful are these error messages for a user? What argument is illegal? 
Is there a way to handle these errors differently than just rethrowing the 
erros and logging the messages?


- Sergio Pena


On March 1, 2018, 6:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65866/
> ---
> 
> (Updated March 1, 2018, 6:19 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Liam

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-07 Thread Steve Moist via Review Board 


> On March 1, 2018, 5:39 p.m., Steve Moist wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
> > Lines 140 (patched)
> > 
> >
> > I don't find stating illegargumentexception to be helpful.  What kind 
> > of illegal argument woould trigger this exception to happen?
> 
> Arjun Mishra wrote:
> See KeyValue class line 37

I mean more express the exception message in a more user friendly way.  What is 
it trying to accomplish here, and why are we specifically logging it as a IAE 
when we could just collapse all the messages into single catch(Exception e) and 
log it there.


- Steve


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


On March 1, 2018, 6:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65866/
> ---
> 
> (Updated March 1, 2018, 6:19 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and 
> Xinran Tinney.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There are a bunch of improvements that should be made to 
> ResourceAuthorizationProvider. For example, exceptions thrown by 
> privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
> hard. 
> 
> We also need to add a lot more logging that needs to be added to related 
> classes
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  7565a34b5 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  09bd9b566 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  447deaf58 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  4e944e5f2 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  ab5560994 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
>  2f8296bfa 
>   
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
>  d338f0edd 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f319 
> 
> 
> Diff: https://reviews.apache.org/r/65866/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-binding/sentry-binding-hive/pom.xml test
> mvn -f sentry-core/sentry-core-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-engine/pom.xml test
> mvn -f sentry-provider/sentry-provider-common/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-01 Thread Arjun Mishra via Review Board 

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

(Updated March 1, 2018, 6:19 p.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and 
Xinran Tinney.


Repository: sentry


Description
---

There are a bunch of improvements that should be made to 
ResourceAuthorizationProvider. For example, exceptions thrown by 
privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
hard. 

We also need to add a lot more logging that needs to be added to related classes


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 7565a34b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b566 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf58 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
 4e944e5f2 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
 ab5560994 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
 2f8296bfa 
  
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
 d338f0edd 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 a9b98f319 


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

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


Testing
---

mvn -f sentry-binding/sentry-binding-hive/pom.xml test
mvn -f sentry-core/sentry-core-common/pom.xml test
mvn -f sentry-policy/sentry-policy-common/pom.xml test
mvn -f sentry-policy/sentry-policy-engine/pom.xml test
mvn -f sentry-provider/sentry-provider-common/pom.xml test


Thanks,

Arjun Mishra

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-01 Thread Arjun Mishra via Review Board 


> On March 1, 2018, 5:39 p.m., Steve Moist wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
> > Lines 140 (patched)
> > 
> >
> > I don't find stating illegargumentexception to be helpful.  What kind 
> > of illegal argument woould trigger this exception to happen?

See KeyValue class line 37


- Arjun


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


On March 1, 2018, 11:47 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65866/
> ---
> 
> (Updated March 1, 2018, 11:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and 
> Xinran Tinney.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There are a bunch of improvements that should be made to 
> ResourceAuthorizationProvider. For example, exceptions thrown by 
> privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
> hard. 
> 
> We also need to add a lot more logging that needs to be added to related 
> classes
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  7565a34b5 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  09bd9b566 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  447deaf58 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  4e944e5f2 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  ab5560994 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
>  2f8296bfa 
>   
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
>  d338f0edd 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f319 
> 
> 
> Diff: https://reviews.apache.org/r/65866/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-binding/sentry-binding-hive/pom.xml test
> mvn -f sentry-core/sentry-core-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-engine/pom.xml test
> mvn -f sentry-provider/sentry-provider-common/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-01 Thread Arjun Mishra via Review Board 

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




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


See KeyValue class line 37


- Arjun Mishra


On March 1, 2018, 11:47 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65866/
> ---
> 
> (Updated March 1, 2018, 11:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and 
> Xinran Tinney.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There are a bunch of improvements that should be made to 
> ResourceAuthorizationProvider. For example, exceptions thrown by 
> privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
> hard. 
> 
> We also need to add a lot more logging that needs to be added to related 
> classes
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  7565a34b5 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  09bd9b566 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  447deaf58 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  4e944e5f2 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  ab5560994 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
>  2f8296bfa 
>   
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
>  d338f0edd 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f319 
> 
> 
> Diff: https://reviews.apache.org/r/65866/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-binding/sentry-binding-hive/pom.xml test
> mvn -f sentry-core/sentry-core-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-common/pom.xml test
> mvn -f sentry-policy/sentry-policy-engine/pom.xml test
> mvn -f sentry-provider/sentry-provider-common/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-01 Thread Arjun Mishra via Review Board 

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

(Updated March 1, 2018, 11:47 a.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and 
Xinran Tinney.


Repository: sentry


Description
---

There are a bunch of improvements that should be made to 
ResourceAuthorizationProvider. For example, exceptions thrown by 
privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
hard. 

We also need to add a lot more logging that needs to be added to related classes


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 7565a34b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b566 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf58 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
 4e944e5f2 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
 ab5560994 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
 2f8296bfa 
  
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
 d338f0edd 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 a9b98f319 


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


Testing (updated)
---

mvn -f sentry-binding/sentry-binding-hive/pom.xml test
mvn -f sentry-core/sentry-core-common/pom.xml test
mvn -f sentry-policy/sentry-policy-common/pom.xml test
mvn -f sentry-provider/sentry-provider-common/pom.xml test


Thanks,

Arjun Mishra

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

2018-03-01 Thread Arjun Mishra via Review Board 

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

(Updated March 1, 2018, 11:41 a.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
kalvagadda, Na Li, Sergio Pena, and Vadim Spector.


Summary (updated)
-

SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and 
improve logging in related classes


Repository: sentry


Description
---

There are a bunch of improvements that should be made to 
ResourceAuthorizationProvider. For example, exceptions thrown by 
privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
hard. 

We also need to add a lot more logging that needs to be added to related classes


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 7565a34b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b566 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf58 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
 4e944e5f2 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
 ab5560994 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
 2f8296bfa 
  
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
 d338f0edd 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 a9b98f319 


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


Testing
---


Thanks,

Arjun Mishra