----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54454/#review161876 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 290) <https://reviews.apache.org/r/54454/#comment233166> Code assumes that there will be atleast from privilege. We have some validations added to make sure of it. I will raise another jira for that. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1045) <https://reviews.apache.org/r/54454/#comment233158> There are no tests using this function. I initially thought of adding tests for them so I mde them visible for tests. I later dropped that idea. I will remove it. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1047) <https://reviews.apache.org/r/54454/#comment233159> There will be alteast one privilage in the set. Any way, this function is called after valid performing below check. if(request.isSetPrivileges() && (!request.getPrivileges().isEmpty())) sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1050) <https://reviews.apache.org/r/54454/#comment233168> I normally follow a convention of having a single return at the end. Do you see any performence issue here? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1057) <https://reviews.apache.org/r/54454/#comment233169> Purpose of both the functions is same. They just differ in the arguments provided. This is a perfect place to use overloaded functions. Why do you think it would be confusing? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1058) <https://reviews.apache.org/r/54454/#comment233165> I wanted this vaidation functions as modular as possible so that they can be re-used. That was the intent behind having seperate functions for TSentryPrivilege and Set<TSentryPrivilege>. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1059) <https://reviews.apache.org/r/54454/#comment233161> privileges set will have al least one element. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1079) <https://reviews.apache.org/r/54454/#comment233155> I agree, this code can be inlined in above function. I have seperated it for reason. This way this utility function are more re-usable. If some part of the code wants to check the mandatory feilds in a single privilege they can just call this function. If is not modular people might come up other functions to vaidate privilege if the validations are to be enchanced in future. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1083) <https://reviews.apache.org/r/54454/#comment233156> Scope of these changes are limited to SENTRY-1547 and SENTRY-1548. We are not really validating the actions here. We have seperate jira to addresss that. - kalyan kumar kalvagadda On Jan. 5, 2017, 4:50 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54454/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2017, 4:50 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > 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 > 898632d > > Diff: https://reviews.apache.org/r/54454/diff/ > > > Testing > ------- > > Verfied the changes using sentry thrift client. > > > Thanks, > > kalyan kumar kalvagadda > >
