----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23794/#review48394 -----------------------------------------------------------
The patch looks mostly good Xiaomeng, I do have a few comments though.. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java <https://reviews.apache.org/r/23794/#comment84956> Any special need for a new Exception ? we could reuse SentryAccessDeniedException rite ? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/23794/#comment84955> Maybe you can use/modify the "getChildPrivileges()" method that is already in SentryStore ? Also, like I mentioned in https://reviews.apache.org/r/23786/ , Is it possible for you to avoid using "privilegeName" ? - Arun Suresh On July 22, 2014, 7:16 a.m., Xiaomeng Huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23794/ > ----------------------------------------------------------- > > (Updated July 22, 2014, 7:16 a.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > A role has a privilege with grant option, he can grant/revoke the privilege > or child privilege to/from other roles. > A role in adminGroups, he can grant privilege to any roles. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > f8491db > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > ff8acdc > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 7637376 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java > 79579c6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java > b97db4b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > a4ae291 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > 838e8d3 > > Diff: https://reviews.apache.org/r/23794/diff/ > > > Testing > ------- > > The unit test is included in the patch. > > > Thanks, > > Xiaomeng Huang > >
