----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67174/#review203371 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1054 (patched) <https://reviews.apache.org/r/67174/#comment285545> Don't we need to provide the implementation for this in this patch? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java Lines 303 (patched) <https://reviews.apache.org/r/67174/#comment285546> Agree, This method might not be needed if the callers just make the correct call instead of using this generic method. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 172-173 (original), 176-178 (patched) <https://reviews.apache.org/r/67174/#comment285548> Have we decided what the decomposition of the ALL when revoking individual actions be? I think we should not fix this issue yet. I thing the discussion is that we won't be able to decompose the ALL to avoid incompatibilities. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 904-906 (patched) <https://reviews.apache.org/r/67174/#comment285565> try/catch are expensive operations in Java. If this alterSentryUserGrantPrivileges() is executed thousands of times with different usernames that do not exist, then it can lower the speed. Should we have a private metod that checks if the username exists, and if not, just add it? Or better a createUserIfNotExist() ? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 785-793 (original) <https://reviews.apache.org/r/67174/#comment285566> I think we should not fix yet. We're still in the decision whether to support decomposing the ALL privilege or just not allow it. There are incompatibilities when decompose it, so we just think it better before fixing it. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 1001-1009 (patched) <https://reviews.apache.org/r/67174/#comment285567> Same comment about using a method that checks if the user exists or createSentryUserIfExists() instead of using try/catch to validate it. Try/catch is expensive. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 4336 (patched) <https://reviews.apache.org/r/67174/#comment285550> noSuchRole() and noSuchGroup() exception messages are similar, can we do the same thing for noSuchUser()? - Sergio Pena On May 17, 2018, 9:41 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67174/ > ----------------------------------------------------------- > > (Updated May 17, 2018, 9:41 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar > kalvagadda, and Sergio Pena. > > > Bugs: sentry-2156 > https://issues.apache.org/jira/browse/sentry-2156 > > > Repository: sentry > > > Description > ------- > > Add functions related to grant/revoke privileges to/from user > > Fix the bugs related to grant/revoke partial privileges. They are caused by > adding fine grained privileges (CRATE, DROP, ALTER) > add test cases for grant/revoke privileges to/from user > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java > 71e9585 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 816cfe1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java > 8a77fc1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > cafe2b5 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 0322cc3 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java > 3e31852 > > > Diff: https://reviews.apache.org/r/67174/diff/3/ > > > Testing > ------- > > all test cases in TestSentryStore passed > > > Thanks, > > Na Li > >
