> On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=2024613#file2024613line303> > > > > Please do not add undocumented public methods. > > Do you really need this? Can callers decide which method to call > > instead - they can call either addUsersFilter or addRolesFilter.
I will add doc for this pub function. THEY can call either function, but the caller has to check entity type and decide. This code block repeats for many times. That is why I decide to have a function for that. > On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 373 (patched) > > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line374> > > > > Is there a value in retrying this? I remove the retry since it is possible the operation may fail when other thread created the user > On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 375 (patched) > > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line376> > > > > Can you use lambda-style instead similar to the way other methods do it? The project I am going to apply this update is in Java7. It is easier for me to port the code if I don't use java 8 feature. > On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 796 (patched) > > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line797> > > > > First sentence should end with a dot. This talks about doing something > > to the entity's privileges, but there is no entity parameter - which one is > > the entity? entityPrivileges is the privileges of the entity. We don't need other info from that entity > On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 803 (patched) > > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line804> > > > > This is a predicate, so probably it should be called isSomething... It is more than predicate. It checks the privilegs for an entity, and return individual privileges that should be removed. I changed the name to checkExistingPrivileges. Hope it is better > On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 812 (patched) > > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line813> > > > > you can use > > > > ```Sets.newHashSet("a", "b", "c")``` Thanks! I decide to use PARTIAL_REVOKE_ACTIONS as input and then remove ALL actions > On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 819 (patched) > > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line820> > > > > It is better to have preconditions check upfront - there is no need to > > do extra work if preconditions fail. > > > > You can also put @NotNull annotation - it will document not null > > requirements and enable automatic checking. I moved the check upfront. I add the comment in the input parameter that is cannot be null. The precondition should have runtime check for null > On May 17, 2018, 9:40 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 858 (patched) > > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line859> > > > > It may be easier to read if you modify the if statement above: > > > > ```if (...) { > > return true; > > } > > > > // handle the more complicated case > > ``` updated - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67174/#review203319 ----------------------------------------------------------- On May 17, 2018, 1:28 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67174/ > ----------------------------------------------------------- > > (Updated May 17, 2018, 1:28 a.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 > > > Diff: https://reviews.apache.org/r/67174/diff/2/ > > > Testing > ------- > > all test cases in TestSentryStore passed > > > Thanks, > > Na Li > >
