----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24961/#review53257 -----------------------------------------------------------
Looks mostly fine. A few of minor comments below. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/24961/#comment92839> I guess if this is a server level privilege, then both will be null. Do we have to add that check as well ? sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/24961/#comment92840> I don't think we should change the sequence of fields. Thrift use that to maintain the backward compatibility. The columnName should be added at the end as 11th field. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/24961/#comment92841> Could you please add some more test cases on partial privileges, eg remove insert from all. That part of code is bit messy, so it would be good to have a bit more tests for that. Thanks! - Prasad Mujumdar On Sept. 4, 2014, 10:23 a.m., Xiaomeng Huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24961/ > ----------------------------------------------------------- > > (Updated Sept. 4, 2014, 10:23 a.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > rename table should get child privileges firstly, then add new child > privileges with new tablename > > > Diffs > ----- > > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java > de35bfa > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryAuthorizable.java > 59418a3 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java > 54b6204 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 33600e9 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > b14616b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryPrivilege.java > 91d3171 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 985a73d > > Diff: https://reviews.apache.org/r/24961/diff/ > > > Testing > ------- > > test case are included. > > > Thanks, > > Xiaomeng Huang > >
