You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by Na Li via Review Board <no...@reviews.apache.org> on 2018/05/17 00:31:42 UTC

Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/
-----------------------------------------------------------

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


Testing
-------

all test cases in TestSentryStore passed


Thanks,

Na Li

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 10:22 p.m., Sergio Pena 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>
> >
> >     Agree, This method might not be needed if the callers just make the correct call instead of using this generic method.
> 
> Na Li wrote:
>     The checking entity types hapen in multiple places. Based on your preference, when the same code blocks are called multiple times, we should have a function to abstract that, so the code is cleaner.

I have removed this function addEntityFilter?


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203371
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 10:22 p.m., Sergio Pena wrote:
> > 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/diff/2/?file=2024614#file2024614line4347>
> >
> >     noSuchRole() and noSuchGroup() exception messages are similar, can we do the same thing for noSuchUser()?
> 
> Na Li wrote:
>     This is exactly what you are asking for
>     
>       private static SentryNoSuchObjectException noSuchUser(String userName) {
>     
>         return new SentryNoSuchObjectException("nonexistent user " + userName);

The message is different, see both group and role:

return new SentryNoSuchObjectException("Role " + roleName);
return new SentryNoSuchObjectException("Group " + groupName);
return new SentryNoSuchObjectException("nonexistent user " + userName);

Shouldn't we have?:

return new SentryNoSuchObjectException("User " + userName);


- Sergio


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203371
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 10:22 p.m., Sergio Pena wrote:
> > 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/diff/2/?file=2024614#file2024614line177>
> >
> >     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.
> 
> Na Li wrote:
>     This code change is not to change behavior, but to fix the bugs in existing behavior. If we decide to change the behavior, and not decompose the ALL, we need to have another jira, and not change the behavior in this jira.

I changed the code, not the partial revoke behaves the same as before


> On May 17, 2018, 10:22 p.m., Sergio Pena wrote:
> > 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/diff/2/?file=2024614#file2024614line946>
> >
> >     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.

the code is changed to keep old behavior for partial revoke


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203371
-----------------------------------------------------------


On May 21, 2018, 10:22 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:22 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/5/
> 
> 
> Testing
> -------
> 
> all test cases in TestSentryStore passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 10:22 p.m., Sergio Pena wrote:
> > 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/diff/2/?file=2024614#file2024614line4347>
> >
> >     noSuchRole() and noSuchGroup() exception messages are similar, can we do the same thing for noSuchUser()?

This is exactly what you are asking for

  private static SentryNoSuchObjectException noSuchUser(String userName) {

    return new SentryNoSuchObjectException("nonexistent user " + userName);


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203371
-----------------------------------------------------------


On May 18, 2018, 3:27 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67174/
> -----------------------------------------------------------
> 
> (Updated May 18, 2018, 3:27 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/4/
> 
> 
> Testing
> -------
> 
> all test cases in TestSentryStore passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 10:22 p.m., Sergio Pena wrote:
> > 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/diff/2/?file=2024612#file2024612line1054>
> >
> >     Don't we need to provide the implementation for this in this patch?

In order to do so, the datastructure for Thrift should be changed to contain the privileges for both role and user. There is another jira for that. Without that change, the returned result still not used and returned to client.


> On May 17, 2018, 10:22 p.m., Sergio Pena 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>
> >
> >     Agree, This method might not be needed if the callers just make the correct call instead of using this generic method.

The checking entity types hapen in multiple places. Based on your preference, when the same code blocks are called multiple times, we should have a function to abstract that, so the code is cleaner.


> On May 17, 2018, 10:22 p.m., Sergio Pena wrote:
> > 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/diff/2/?file=2024614#file2024614line177>
> >
> >     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.

This code change is not to change behavior, but to fix the bugs in existing behavior. If we decide to change the behavior, and not decompose the ALL, we need to have another jira, and not change the behavior in this jira.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203371
-----------------------------------------------------------


On May 18, 2018, 3:27 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67174/
> -----------------------------------------------------------
> 
> (Updated May 18, 2018, 3:27 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/4/
> 
> 
> Testing
> -------
> 
> all test cases in TestSentryStore passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.

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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 23, 2018, 5:03 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 852-860 (patched)
> > <https://reviews.apache.org/r/67174/diff/6/?file=2026762#file2026762line853>
> >
> >     Lina, what if we keep the original code instead of improve it? I see the original code is:
> >     
> >     TSentryPrivilege tNotAll = new TSentryPrivilege(privilege);
> >             tNotAll.setAction(AccessConstants.SELECT);
> >             MSentryPrivilege mSelect = getMSentryPrivilege(tNotAll, pm);
> >             tNotAll.setAction(AccessConstants.INSERT);
> >             MSentryPrivilege mInsert = getMSentryPrivilege(tNotAll, pm);
> >             
> >     I'd rather stayed with the original code until we decide what we're going to do with this part of the code. Also, this would allow us to understand the improvement and make comments about the improvement. This patch is related to the user privilege methods only.

I revert my changes and will use original code


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203668
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203668
-----------------------------------------------------------




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 852-860 (patched)
<https://reviews.apache.org/r/67174/#comment285952>

    Lina, what if we keep the original code instead of improve it? I see the original code is:
    
    TSentryPrivilege tNotAll = new TSentryPrivilege(privilege);
            tNotAll.setAction(AccessConstants.SELECT);
            MSentryPrivilege mSelect = getMSentryPrivilege(tNotAll, pm);
            tNotAll.setAction(AccessConstants.INSERT);
            MSentryPrivilege mInsert = getMSentryPrivilege(tNotAll, pm);
            
    I'd rather stayed with the original code until we decide what we're going to do with this part of the code. Also, this would allow us to understand the improvement and make comments about the improvement. This patch is related to the user privilege methods only.


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.

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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 23, 2018, 9:02 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 169-171 (original), 172-176 (patched)
> > <https://reviews.apache.org/r/67174/diff/6/?file=2026762#file2026762line173>
> >
> >     Changes to these comments is not relavent with new patch.

revert the change


> On May 23, 2018, 9:02 p.m., kalyan kumar kalvagadda 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/6/?file=2026762#file2026762line374>
> >
> >     Can you use lambda's instead. Please refer places where executeTransaction and executeTransactionWithRetry are called?

changed


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203598
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203598
-----------------------------------------------------------




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 169-171 (original), 172-176 (patched)
<https://reviews.apache.org/r/67174/#comment285884>

    Changes to these comments is not relavent with new patch.



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/#comment285885>

    Can you use lambda's instead. Please refer places where executeTransaction and executeTransactionWithRetry are called?


- kalyan kumar kalvagadda


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203782
-----------------------------------------------------------


Ship it!




Looks good.

- kalyan kumar kalvagadda


On May 24, 2018, 12:03 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67174/
> -----------------------------------------------------------
> 
> (Updated May 24, 2018, 12:03 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 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/7/
> 
> 
> Testing
> -------
> 
> all test cases in TestSentryStore passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203769
-----------------------------------------------------------


Ship it!




Thanks Lina.

- Sergio Pena


On May 24, 2018, 12:03 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67174/
> -----------------------------------------------------------
> 
> (Updated May 24, 2018, 12:03 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 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/7/
> 
> 
> Testing
> -------
> 
> all test cases in TestSentryStore passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/
-----------------------------------------------------------

(Updated May 24, 2018, 12:03 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 (updated)
-----

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

Changes: https://reviews.apache.org/r/67174/diff/6-7/


Testing
-------

all test cases in TestSentryStore passed


Thanks,

Na Li

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 23, 2018, 5:09 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 785-788 (original), 897-905 (patched)
> > <https://reviews.apache.org/r/67174/diff/6/?file=2026762#file2026762line915>
> >
> >     can't this code be part of the checkExistingPrivileges instead of putting it here? Like the old code does. 
> >     
> >     Also, if done that way, we chould rename the checkExistingPrivileges() and description to reflect the reason of the method.

I will remove checkExistingPrivileges, so this command won't apply any more


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203669
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203669
-----------------------------------------------------------




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 785-788 (original), 897-905 (patched)
<https://reviews.apache.org/r/67174/#comment285953>

    can't this code be part of the checkExistingPrivileges instead of putting it here? Like the old code does. 
    
    Also, if done that way, we chould rename the checkExistingPrivileges() and description to reflect the reason of the method.


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
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 (updated)
-----

  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/

Changes: https://reviews.apache.org/r/67174/diff/5-6/


Testing
-------

all test cases in TestSentryStore passed


Thanks,

Na Li

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/
-----------------------------------------------------------

(Updated May 21, 2018, 10:22 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 (updated)
-----

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

Changes: https://reviews.apache.org/r/67174/diff/4-5/


Testing
-------

all test cases in TestSentryStore passed


Thanks,

Na Li

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/
-----------------------------------------------------------

(Updated May 18, 2018, 3:27 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 (updated)
-----

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

Changes: https://reviews.apache.org/r/67174/diff/3-4/


Testing
-------

all test cases in TestSentryStore passed


Thanks,

Na Li

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
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 (updated)
-----

  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/

Changes: https://reviews.apache.org/r/67174/diff/2-3/


Testing
-------

all test cases in TestSentryStore passed


Thanks,

Na Li

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 12:36 p.m., kalyan kumar kalvagadda wrote:
> > In general note can't you seperate patches for below?
> > 1.Add functions related to grant/revoke privileges to/from user
> > 2.Fix the bugs related to grant/revoke partial privileges. They are caused by adding fine grained privileges (CRATE, DROP, ALTER)

It would take much longer time to seperate the patch. Since I need to merge the uer-privilege and role-privilege functions, I have to rewrite some functions. It is hard to have a new function with bug in it. and call it a patch.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203327
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203327
-----------------------------------------------------------



In general note can't you seperate patches for below?
1.Add functions related to grant/revoke privileges to/from user
2.Fix the bugs related to grant/revoke partial privileges. They are caused by adding fine grained privileges (CRATE, DROP, ALTER)

- kalyan kumar kalvagadda


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 4:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 169-173 (original), 171-178 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line172>
> >
> >     This can be in seperate patch.

There are more places involved


> On May 17, 2018, 4:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 805 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line806>
> >
> >     Can you make this API interface simple? Currently there are two inputs from this method to the caller.
> >     1. Return value
> >     2. privilegesToRemove

what's your suggestion on returning two outuputs?


> On May 17, 2018, 4:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 835 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line836>
> >
> >     Isn't this a duplicate DB operation?
> >     
> >     entityPrivileges which are passed as arguments is retrieved from the database. You can simplt try to find tNotAll in entityPrivileges.

will update


> On May 17, 2018, 4:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 1039 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line1049>
> >
> >     Most of the private methods could be generic.

but the privilege function names are not generic


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203329
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 4:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 169-173 (original), 171-178 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line172>
> >
> >     This can be in seperate patch.
> 
> Na Li wrote:
>     There are more places involved

When I rewrite the code to combine both role and user for privileges, the existing bugs are fixed. 

1) If we just have another patch for PARTIAL_REVOKE_ACTIONS and test cases, the patch does not really reflect the real code change
2) Or, I need to purposely change the rewritten working codee to make it contains the original bug. It takes more time to do so and does not make sense for me to have one patch contains rewritten code but keeps bugs I know, and another patch to fix those bugs.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203329
-----------------------------------------------------------


On May 18, 2018, 3:27 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67174/
> -----------------------------------------------------------
> 
> (Updated May 18, 2018, 3:27 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/4/
> 
> 
> Testing
> -------
> 
> all test cases in TestSentryStore passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 4:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 169-173 (original), 171-178 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line172>
> >
> >     This can be in seperate patch.
> 
> Na Li wrote:
>     There are more places involved
> 
> Na Li wrote:
>     When I rewrite the code to combine both role and user for privileges, the existing bugs are fixed. 
>     
>     1) If we just have another patch for PARTIAL_REVOKE_ACTIONS and test cases, the patch does not really reflect the real code change
>     2) Or, I need to purposely change the rewritten working codee to make it contains the original bug. It takes more time to do so and does not make sense for me to have one patch contains rewritten code but keeps bugs I know, and another patch to fix those bugs.

I remove the code that will decompose all to include {drop, create, alter}. So partial revoke behaves the same as before.


> On May 17, 2018, 4:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 805 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024614#file2024614line806>
> >
> >     Can you make this API interface simple? Currently there are two inputs from this method to the caller.
> >     1. Return value
> >     2. privilegesToRemove
> 
> Na Li wrote:
>     what's your suggestion on returning two outuputs?

As I talked with you, there are following possible scenarios to cover
1) The processing should not continue. For example, the user has "ALL" privilege on the object. granting any individual privilege is no-op
2) The processing should continue. privilegesToRemove is empty. For example, user has no privileges
3) The processing should continue. privilegesToRemove is not empty. For example, user has "insert"  and then granted "ALL"

We cannot cover all these scenario using privilegesToRemove only.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203329
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203329
-----------------------------------------------------------




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 169-173 (original), 171-178 (patched)
<https://reviews.apache.org/r/67174/#comment285458>

    This can be in seperate patch.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 805 (patched)
<https://reviews.apache.org/r/67174/#comment285480>

    Can you make this API interface simple? Currently there are two inputs from this method to the caller.
    1. Return value
    2. privilegesToRemove



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 835 (patched)
<https://reviews.apache.org/r/67174/#comment285459>

    Isn't this a duplicate DB operation?
    
    entityPrivileges which are passed as arguments is retrieved from the database. You can simplt try to find tNotAll in entityPrivileges.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 1039 (patched)
<https://reviews.apache.org/r/67174/#comment285479>

    Most of the private methods could be generic.


- kalyan kumar kalvagadda


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 2:45 p.m., Arjun Mishra wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
> > Lines 253 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024611#file2024611line253>
> >
> >     Can we also have a type GROUP, and UNKNOWN to be consistent with HivePrincipalType?
> 
> Na Li wrote:
>     The purpose of entity type is for associating privileges. We don't have a plan to assign privilege to group or unknown.

Arjun, We don't support GROUP and UNKNOWN yet, any reasons we want to add them here?


- Sergio


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203312
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> On May 17, 2018, 2:45 p.m., Arjun Mishra wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
> > Lines 253 (patched)
> > <https://reviews.apache.org/r/67174/diff/2/?file=2024611#file2024611line253>
> >
> >     Can we also have a type GROUP, and UNKNOWN to be consistent with HivePrincipalType?

The purpose of entity type is for associating privileges. We don't have a plan to assign privilege to group or unknown.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203312
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203312
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
Lines 253 (patched)
<https://reviews.apache.org/r/67174/#comment285420>

    Can we also have a type GROUP, and UNKNOWN to be consistent with HivePrincipalType?


- Arjun Mishra


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.


> 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?
> 
> Na Li wrote:
>     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.

changed to lambda.


> 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...
> 
> Na Li wrote:
>     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

This function is removed. So the command does not apply any more.


- Na


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203319
-----------------------------------------------------------


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67174/#review203319
-----------------------------------------------------------




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/#comment285425>

    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.



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/#comment285426>

    Is there a value in retrying this?



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/#comment285427>

    Can you use lambda-style instead similar to the way other methods do it?



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/#comment285430>

    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?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 798 (patched)
<https://reviews.apache.org/r/67174/#comment285431>

    typo: input.
    Are existing privileges just removed or replaced with ALL?



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/#comment285432>

    This is a predicate, so probably it should be called isSomething...



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/#comment285434>

    you can use 
    
    ```Sets.newHashSet("a", "b", "c")```



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/#comment285435>

    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.



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/#comment285436>

    It may be easier to read if you modify the if statement above:
    
    ```if (...) {
           return true;
       }
       
       // handle the more complicated case
    ```


- Alexander Kolbasov


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

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

Posted by Na Li via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
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 (updated)
-----

  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/

Changes: https://reviews.apache.org/r/67174/diff/1-2/


Testing
-------

all test cases in TestSentryStore passed


Thanks,

Na Li