> On May 22, 2018, 10:29 p.m., Sergio Pena wrote: > > 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/diff/6/?file=2026762#file2026762line802> > > > > 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?
This is performance optimization. I will remove this change and fire another jira for performance improvement > On May 22, 2018, 10:29 p.m., Sergio Pena wrote: > > 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/diff/6/?file=2026762#file2026762line826> > > > > 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? this function is removed. comment does not apply any more > On May 22, 2018, 10:29 p.m., Sergio Pena wrote: > > 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/diff/6/?file=2026762#file2026762line932> > > > > This is not persisted in the original method. Why is it needed now? > > What is it fixed? It is by code inspection. mRole may be changed as some of its existing privileges may be removed. However since there is no test failure without it, I will remove it for now. As long all involed privileges are persisted, not persist mRole is OK. > On May 22, 2018, 10:29 p.m., Sergio Pena wrote: > > 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/diff/6/?file=2026762#file2026762line1247> > > > > 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() Yes. I add it for alterSentryUserRevokePrivilegeCore - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67174/#review203600 ----------------------------------------------------------- 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 > >
