> On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 1052 > > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1052> > > > > I normally follow a convention of having a single return at the end. Do > > you see any performence issue here?
It isn't about performance but baout readability and simplicity. With return you know that you exit the function, otherwise you need to mentally follow what happens next. What benefit do you get from a single return at the end? > On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1589127#file1589127line1059> > > > > 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? Purpose is quite diffetent - one validates a single privilege, another validates multiple privileges. In this case using the same name for both is just confusing. > On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 1060 > > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1060> > > > > 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>. Can you clarify how do you envision such reuse? In what context it would be reused? Anyway, if you think that it is better to split this into two functions, this is ok as well. > On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 1081 > > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1081> > > > > 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. This is Ok to have it in separate functions if you think that it is useful - but what code do envision will be using it? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54454/#review161876 ----------------------------------------------------------- On Jan. 17, 2017, 7:28 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54454/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2017, 7:28 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 > 898632ddef6ac5eec7b0e240c596f57f427fda9d > > Diff: https://reviews.apache.org/r/54454/diff/ > > > Testing > ------- > > Verfied the changes using sentry thrift client. > > > Thanks, > > kalyan kumar kalvagadda > >
