----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28507/#review63192 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/28507/#comment105380> Hi, I think we don't need to get all child privileges and then do the filter. We can just add a query filter like: if parent's action is not ALL filters.append(" && action == \"" + parent.getAction() + "\""); This approach make the query clearly and improve performance. Hi Lenni, the patch looks fine me. If you use implies approach, a bug SENTRY-553 may block your test case in TestDatabaseProvider#testRevokeAllOnServer, I have fixed it, if you have time, please have a look. I also have another approach to add a query filter for action and comment in your patch, I think it also can work. Other comments, there are some extra whitespaces in your patch. - Xiaomeng Huang On 十一月 27, 2014, 7:18 a.m., Lenni Kuff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28507/ > ----------------------------------------------------------- > > (Updated 十一月 27, 2014, 7:18 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > A revoke on a parent authorizeable was not taking into account the privilege > action when removing child privileges. This mean that a REVOKE SELECT ON > DATABASE would also delete privileges with an action of ALL or INSERT on > child tables/columns. This fix is to verify that each of the child privileges > collected "implies" the parent privilege. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 073bb33 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > e8be81f > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java > 3189955 > > Diff: https://reviews.apache.org/r/28507/diff/ > > > Testing > ------- > > Added new tests. > > > Thanks, > > Lenni Kuff > >
