----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67174/#review203600 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 801-812 (patched) <https://reviews.apache.org/r/67174/#comment285886> This method does the same thing as the contains() method of the Set<>. boolean contains(intputPrivilege); Can we call the contains() method instead of writing our own? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 825 (patched) <https://reviews.apache.org/r/67174/#comment285887> This method does more than checking. Should we rename it to something that reflects what it is really doing? checkExistingPrivileges() sounds that it will check only, and return true or false if the check passed? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 853 (patched) <https://reviews.apache.org/r/67174/#comment285891> This initialization can be done outside of the for() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 914 (patched) <https://reviews.apache.org/r/67174/#comment285888> This is not persisted in the original method. Why is it needed now? What is it fixed? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 937 (patched) <https://reviews.apache.org/r/67174/#comment285892> proeed -> proceed sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 1080 (patched) <https://reviews.apache.org/r/67174/#comment285893> Why is this 'retrieve' needed? I just wonder because I don't see this called in getMSentryRoleByName(). sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 1229-1230 (patched) <https://reviews.apache.org/r/67174/#comment285894> is this required? if(tPrivilege.getPrivilegeScope().equalsIgnoreCase(PrivilegeScope.URI.name()) && StringUtils.isBlank(tPrivilege.getURI())) { throw new SentryInvalidInputException("cannot revoke URI privileges from Null or EMPTY location"); } I saw it on the alterSentryRoleRevokePrivilegeCore() - Sergio Pena On May 21, 2018, 10:48 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67174/ > ----------------------------------------------------------- > > (Updated May 21, 2018, 10:48 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 > 56c506b > > 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/6/ > > > Testing > ------- > > all test cases in TestSentryStore passed > > > Thanks, > > Na Li > >
