Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

[Alexander Kolbasov <ak...@gmail.com>]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 36 (patched)
<https://reviews.apache.org/r/54454/#comment239527>

    There is no reason this should be a singleton. In fact, it should only have static methods. Same goes for other validator classes.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java
Lines 32 (patched)
<https://reviews.apache.org/r/54454/#comment239525>

    Looking at the way it is used and defined I don't see any value in creating sucb abstract class. The validate() method can't be generalized since you can't subclass thrift methods, so it isn't very useful. The only thing you need to share is the validateVersion method, so you can just have a class that implements that as a static method - and there is no need for inheritance at all.


- Alexander Kolbasov


On Feb. 28, 2017, 2:21 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 2:21 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1548
>     https://issues.apache.org/jira/browse/SENTRY-1548
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
> 
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
> 
> 
> Diffs
> -----
> 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION 
>   sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/54454/diff/6/
> 
> 
> Testing
> -------
> 
> Verfied the changes using sentry thrift client.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>