----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54454/#review162568 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 281) <https://reviews.apache.org/r/54454/#comment233880> Please add a space between // and a comment sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 283) <https://reviews.apache.org/r/54454/#comment233881> Other code here uses convention if (condition) { ... } 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/#comment233882> The code knows what exactly is missing - so why say that server name [OR] Action are missing? Why not just say what exactly is missing? ALso this line is too long sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 288) <https://reviews.apache.org/r/54454/#comment233884> How do you know that the value is UNSET? May be it was 42? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 289) <https://reviews.apache.org/r/54454/#comment233883> This line is too long. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 359) <https://reviews.apache.org/r/54454/#comment233886> Same comments as above. 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/#comment233888> Would it be useful to allow any iterable collection rather than a set? Please document what is 'validation' 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/#comment233889> A boolean value is, by definition, true/false. You can just say @return true if the validation is successfull, false otherwise That said, this interface makes it difficult to tell exactly what is wrong - you can't pass this information fro the validator. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1056) <https://reviews.apache.org/r/54454/#comment233890> All these validators can be private 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/#comment233891> It will be shorter and cleaner if you just remove the ret boolean and return the needed values directly sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1073) <https://reviews.apache.org/r/54454/#comment233892> As we discussed earlier, use of overloading here just creates confusion sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1075) <https://reviews.apache.org/r/54454/#comment233893> See http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-137265.html#333 A better way to write this is return condition Will Thrift validate that the grant option can't have a value like 42? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1082) <https://reviews.apache.org/r/54454/#comment233894> Please describe which fields are mandatory sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1084) <https://reviews.apache.org/r/54454/#comment233895> See the comment above about boolean return values sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1089) <https://reviews.apache.org/r/54454/#comment233896> See comment above about direct returns sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1102) <https://reviews.apache.org/r/54454/#comment233897> This interface makes it difficult to tell what exact field is missing sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1108) <https://reviews.apache.org/r/54454/#comment233898> This is better written as return condition sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1110) <https://reviews.apache.org/r/54454/#comment233900> It is usuallu better to use isEmpty instead of length() - Alexander Kolbasov 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 > >
