> On July 22, 2014, 6:37 p.m., Arun Suresh wrote: > > The patch looks mostly good Xiaomeng, I do have a few comments though..
Hi, Arun Very appreciate your comments! Let's be free to talk about them! > On July 22, 2014, 6:37 p.m., Arun Suresh wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java, > > line 22 > > <https://reviews.apache.org/r/23794/diff/1/?file=638845#file638845line22> > > > > Any special need for a new Exception ? we could reuse > > SentryAccessDeniedException rite ? Hi, I think it's a little different from AccessDeniedException. In my opinion, AccessDeniedException means the user has no access permission on this privilege. And NoGrantOptionException means the user may has access permission, but no grant permission. We do grant/revoke check before grant/revoke, and NoGrantOptionException only be throw in grant/revoke check. > On July 22, 2014, 6:37 p.m., Arun Suresh wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, > > line 212 > > <https://reviews.apache.org/r/23794/diff/1/?file=638846#file638846line212> > > > > 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" ? I tried to reuse getChildPrivileges, but I found it is not my want. getChildPrivileges() get child privileges of parent from database. e.g. parent = {server1+db1} getChildPrivileges will return [{server1+db1+tb1},{server1+db1+tb2}] However, hasChildPrivilege is just judge whether other privilege is a child of this privilege. It don't need to access database. And I also judge whether the action is a child. And I will modify my patch to avoid using "privilegeName" based on SENTRY-339. - Xiaomeng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23794/#review48394 ----------------------------------------------------------- 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 > >
