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