> On 十二月 1, 2014, 8:16 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java, > > lines 58-60 > > <https://reviews.apache.org/r/25808/diff/3/?file=705107#file705107line58> > > > > Why initialize to empty strings?
I will remove the intialize emtpy strings > On 十二月 1, 2014, 8:16 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java, > > line 61 > > <https://reviews.apache.org/r/25808/diff/3/?file=705107#file705107line61> > > > > Can scope be deduced? I agree with you. I will change it. Thanks for your comments > On 十二月 1, 2014, 8:16 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java, > > line 173 > > <https://reviews.apache.org/r/25808/diff/3/?file=705107#file705107line173> > > > > Looks like we only allow strict heirarchy? That is, can level =1 be not > > null when level = 0 is null? Yes, the generic model only supports strict hierarchy authorizables. I will add a check function for the authorizables. > On 十二月 1, 2014, 8:16 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java, > > line 294 > > <https://reviews.apache.org/r/25808/diff/3/?file=705107#file705107line294> > > > > Do you think we will need to support any case sensitive authorizables? I will fixed it > On 十二月 1, 2014, 8:16 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java, > > line 371 > > <https://reviews.apache.org/r/25808/diff/3/?file=705107#file705107line371> > > > > A comment on why we might need a partial filter excluding scope and > > action? I add a explain comment for the filter. You can see it. Thanks for your comments On 十二月 1, 2014, 8:16 p.m., shen guoquan wrote: > > Thanks for the work Guoquan! Over all looks good to me, some minor comments > > below. Also, we will need to add unit tests for MSentryGMPrivilege and add > > new tests cases for MSentryRole exercising new fields. As they will fail > > without the underlying jdo changes, feel free to file a follow on jira and > > mark it as a sub task for this feature. Thanks! > > shen guoquan wrote: > Thanks for your review. I will fire a jira to add some tests. Thanks, > Sravya. Hey, sravya. I add some unit tests for MsentryGMPrivilege and MsentryRole. You can see my new patch. It not only changed the patch accroding to your comments, but also fixed some issue logic problem and add new unit tests. Please help me reviwing my new patch if you have some free time. Thanks a lot for helpng me. - shen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25808/#review55067 ----------------------------------------------------------- On 十二月 5, 2014, 1:10 p.m., shen guoquan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25808/ > ----------------------------------------------------------- > > (Updated 十二月 5, 2014, 1:10 p.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > Currently the metadata infrastructure in Sentry only supports database > structure authorize model such as Hive/Impala.In order to support other > no-database structure component like Solr, it needs to extend the metadata > infrastructure. Considering the compatibility and not affect the previous > functionality, it is better to add a new privilege in metadata rather than > modifying the original privilege. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > bca9fb9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 594201f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryGMPrivilege.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/25808/diff/ > > > Testing > ------- > > > Thanks, > > shen guoquan > >
