----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54454/#review159250 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 280) <https://reviews.apache.org/r/54454/#comment230195> can getPrivileges() return null? 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/#comment230196> This is the same code as below - can it be handled by a common func? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 282) <https://reviews.apache.org/r/54454/#comment230197> if (!validateGrantOption(request.getPrivilege()) { ... } There is no point in comparing booleans to true/false 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/#comment230198> can this be moe specific like "grant option is unset" or something like this? 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/#comment230199> else if { } sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1066) <https://reviews.apache.org/r/54454/#comment230194> It is a bit confusing for these two functions to share the same name. Also, given that this can be simply written as return privilege.getGrantOption() != TSentryGrantOption.UNSET do we need a function for that at all? - Alexander Kolbasov On Dec. 14, 2016, 10:38 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54454/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2016, 10:38 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 have made changes assuming that grant option is either true/false removing > unset. > Also, added code so that sentry server could validate the TSentryPrivilege > object constructed from the Thrift message received client. If the validation > is failed exception is raised and appropriate error is message is sent. > > > 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 > >
