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/31 15:49:03 UTC
Review Request 67402: SENTRY-2244: alterSentryRoleGrantPrivilegeCore
can avoid extra query to Database
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67402/
-----------------------------------------------------------
Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
Bugs: sentry-2244
https://issues.apache.org/jira/browse/sentry-2244
Repository: sentry
Description
-------
When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
Diff: https://reviews.apache.org/r/67402/diff/1/
Testing
-------
test cases in TestSentryStore succeeded
Thanks,
Na Li
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On May 31, 2018, 9:25 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 818 (original), 841 (patched)
> > <https://reviews.apache.org/r/67402/diff/1/?file=2033279#file2033279line841>
> >
> > Should we do the same check and removal from users as well?
Yes. It is already done.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67402/#review204152
-----------------------------------------------------------
On June 4, 2018, 3:29 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:29 a.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/2/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244:
alterSentryRoleGrantPrivilegeCore can avoid extra query to Database
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/67402/#review204152
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 818 (original), 841 (patched)
<https://reviews.apache.org/r/67402/#comment286541>
Should we do the same check and removal from users as well?
- Sergio Pena
On May 31, 2018, 3:49 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated May 31, 2018, 3:49 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/1/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On May 31, 2018, 9:20 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 792 (original), 813 (patched)
> > <https://reviews.apache.org/r/67402/diff/1/?file=2033279#file2033279line813>
> >
> > Is there some tests that verify this method or the parent public method? We need to make sure this new logic works as expected.
The existing tests verify that the change does not break the functionalities.
The change is in private functions. It is performance improvement and does not change behavior.
If I change the implementation of findMatchPrivilege to return inputPrivilege insterad of matching privilege in entityPrivileges, 24 tests failed, like testGrantRevokePrivilege, testGrantCheckWithGroupAndUser etc. This shows if the behavior changes, many tests will fail.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67402/#review204151
-----------------------------------------------------------
On June 4, 2018, 2:57 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 2:57 a.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/1/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On May 31, 2018, 9:20 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 814 (original), 835-836 (patched)
> > <https://reviews.apache.org/r/67402/diff/1/?file=2033279#file2033279line835>
> >
> > Shoult it be better to move this method to the MSentryRole class instead?
> >
> > This would look better:
> >
> > MSentryPrivilege mSelect =
> > mRole.findPrivilege(convertToMSentryPrivilege(tNotAll));
No. findMatchPrivilege is not specific to a role or a user. It is generial function with input of list of privileges, and a privilege to match, and output the matched privilege in the list.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67402/#review204151
-----------------------------------------------------------
On June 4, 2018, 3:29 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:29 a.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/2/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244:
alterSentryRoleGrantPrivilegeCore can avoid extra query to Database
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/67402/#review204151
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 793 (patched)
<https://reviews.apache.org/r/67402/#comment286539>
typo: Privielge
Also, add more information about why this is done this way instead of using the contains(). You can mention that the inputPrivilege contains all fields except the roles and users information and we are finding the privilege with all users and roles that matches the inputPrivilege.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 792 (original), 813 (patched)
<https://reviews.apache.org/r/67402/#comment286540>
Is there some tests that verify this method or the parent public method? We need to make sure this new logic works as expected.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 814 (original), 835-836 (patched)
<https://reviews.apache.org/r/67402/#comment286538>
Shoult it be better to move this method to the MSentryRole class instead?
This would look better:
MSentryPrivilege mSelect =
mRole.findPrivilege(convertToMSentryPrivilege(tNotAll));
- Sergio Pena
On May 31, 2018, 3:49 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated May 31, 2018, 3:49 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/1/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On June 6, 2018, 2:28 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 817 (original), 843 (patched)
> > <https://reviews.apache.org/r/67402/diff/2/?file=2034813#file2034813line843>
> >
> > How is findMatchPrivileges different from mRole.getPrivileges().contains()? why don't we call
> > mRole.getPrivileges().contains(convertToMSentryPrivilege(tNotAll))?
mRole.getPrivileges().contains() returns boolean value: yes or no. it does not return MSentryPrivilege. That is why I cannot use mRole.getPrivileges().contains(). Do you know any buildin function I can use to get a maching element from Set<MSentryPrivilege>?
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67402/#review204350
-----------------------------------------------------------
On June 4, 2018, 3:29 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:29 a.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/2/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
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/67402/#review204350
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 817 (original), 843 (patched)
<https://reviews.apache.org/r/67402/#comment286824>
How is findMatchPrivileges different from mRole.getPrivileges().contains()? why don't we call
mRole.getPrivileges().contains(convertToMSentryPrivilege(tNotAll))?
- kalyan kumar kalvagadda
On June 4, 2018, 3:29 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:29 a.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/2/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
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/67402/#review204348
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On June 4, 2018, 3:29 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:29 a.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/2/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
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/67402/#review204407
-----------------------------------------------------------
Ship it!
Looks good.
- kalyan kumar kalvagadda
On June 4, 2018, 3:29 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67402/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:29 a.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
>
>
> Bugs: sentry-2244
> https://issues.apache.org/jira/browse/sentry-2244
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
>
>
> Diff: https://reviews.apache.org/r/67402/diff/2/
>
>
> Testing
> -------
>
> test cases in TestSentryStore succeeded
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
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/67402/
-----------------------------------------------------------
(Updated June 4, 2018, 3:29 a.m.)
Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
Bugs: sentry-2244
https://issues.apache.org/jira/browse/sentry-2244
Repository: sentry
Description
-------
When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
Diff: https://reviews.apache.org/r/67402/diff/2/
Changes: https://reviews.apache.org/r/67402/diff/1-2/
Testing
-------
test cases in TestSentryStore succeeded
Thanks,
Na Li
Re: Review Request 67402: SENTRY-2244: Alter sentry role or user at
granting privilege can avoid extra query to database
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/67402/
-----------------------------------------------------------
(Updated June 4, 2018, 2:57 a.m.)
Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
Summary (updated)
-----------------
SENTRY-2244: Alter sentry role or user at granting privilege can avoid extra query to database
Bugs: sentry-2244
https://issues.apache.org/jira/browse/sentry-2244
Repository: sentry
Description
-------
When granting privilege to a user or role, sentry does not need to get individual privilege with specific action from DB again after all its associated privileges are fetched from DB. We just just find the corresponding privilege from list to avoid extra query to DB
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 5932335
Diff: https://reviews.apache.org/r/67402/diff/1/
Testing
-------
test cases in TestSentryStore succeeded
Thanks,
Na Li