----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54454/#review160654 -----------------------------------------------------------
The change seems to check for other privilege correctness in addition to UNSET grant option - are you adding a fix for another problem here as well? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 285) <https://reviews.apache.org/r/54454/#comment231813> It would be helpful to report what exactly is missing sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 286) <https://reviews.apache.org/r/54454/#comment231814> You don't need else clause here since the throw above will terminate, so you can just write this as two ifs. 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/#comment231815> Do we need to do anything here if the privilege set is empty? 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/#comment231805> Is there a test that uses it? Why is it visible for testing? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1046) <https://reviews.apache.org/r/54454/#comment231804> Please add javadoc for this function 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/#comment231807> Can privileges be null here? 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/#comment231803> Why not just "return false"? 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/#comment231800> Use of overloading here is confusing - can you choose different names for these functions? 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/#comment231801> A simple return privilege.getGrantOption() != TSentryGrantOption.UNSET works just fine. And given that it is now a one-liner - do we really need a function for it? This will remove the problem with overloaded names as well. 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/#comment231808> Can privilege be null here? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1064) <https://reviews.apache.org/r/54454/#comment231806> Same comments as for validateGrantOption above. 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/#comment231809> Can it really be null? This can be inlined directly in checkforMandatoryPrivilegeFeilds() above as privilege.getServerName() != null && privilege.getServerName().trim().length() > 0 && privilege.getAction() != null && privilege.getAction().trim().length() > 0) 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/#comment231811> Do we allow any action as long as it is non-empty? Other code makes certain assumption about valid set of actions. - Alexander Kolbasov 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 > >
