> 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
> 
>

Reply via email to