You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arjun Mishra via Review Board <no...@reviews.apache.org> on 2018/08/16 19:55:21 UTC
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/
-----------------------------------------------------------
(Updated Aug. 16, 2018, 7:55 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
Summary (updated)
-----------------
SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction
Bugs: SENTRY-1944
https://issues.apache.org/jira/browse/SENTRY-1944
Repository: sentry
Description
-------
When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
It may be possible to optimize it.
Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
Attach one or more files to this issue
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
Diff: https://reviews.apache.org/r/64661/diff/5/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207432
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
Lines 144 (patched)
<https://reviews.apache.org/r/64661/#comment290827>
I would rather add the function
Set<TSentryRole> getRolesByGroups(String component, Set<String> groups) throws Exception;
Then you can call SentryStore.getTSentryRolesByGroupName() to get the result
- Na Li
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 16, 2018, 8:18 p.m., Sergio Pena wrote:
> > What's the expected performance improvement we're getting with this? Have you run tests to check how much time we're saving?
Hey Sergio I did benchmark tests, see first comment in the ticket: https://issues.apache.org/jira/browse/SENTRY-1944?focusedCommentId=16293340&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16293340
Can do a repeat of those tests
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207430
-----------------------------------------------------------
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Aug. 16, 2018, 8:18 p.m., Sergio Pena wrote:
> > What's the expected performance improvement we're getting with this? Have you run tests to check how much time we're saving?
>
> Arjun Mishra wrote:
> Hey Sergio I did benchmark tests, see first comment in the ticket: https://issues.apache.org/jira/browse/SENTRY-1944?focusedCommentId=16293340&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16293340
>
> Can do a repeat of those tests
Arjun,
Sergio may mean the performance improvement for list_sentry_roles_by_group()
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207430
-----------------------------------------------------------
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207430
-----------------------------------------------------------
What's the expected performance improvement we're getting with this? Have you run tests to check how much time we're saving?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
Line 590 (original), 590 (patched)
<https://reviews.apache.org/r/64661/#comment290820>
Code convention: Need a space to separate the variable name from the type.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 244-246 (original), 249-252 (patched)
<https://reviews.apache.org/r/64661/#comment290819>
Is it necessary to throw a NullPointerException if roles is null?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 278 (patched)
<https://reviews.apache.org/r/64661/#comment290821>
Is it necessary to throw a NPE if roles is null?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 294 (patched)
<https://reviews.apache.org/r/64661/#comment290822>
You can construct the hasmap with an initial capacitity of roles.size() to avoid the hashmap to be resized during the for loop.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 295-297 (patched)
<https://reviews.apache.org/r/64661/#comment290823>
Code convention: Space between for and ()
Btw, this has 2 loops. I think we can avoid using a 2nd loop by making the query to request all roles instead of all groups. That way you just walk through the list of roles and put each role and its groups in the map (this saves one loop and the containsKey and get calls).
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
Lines 139-142 (patched)
<https://reviews.apache.org/r/64661/#comment290824>
Need to fill information of @param, @return and @throws. I usually put in @throws why I am expecting this exception.
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java
Lines 292 (patched)
<https://reviews.apache.org/r/64661/#comment290825>
Coding convention: space >mockMap.
- Sergio Pena
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:20 a.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
> > Lines 144 (patched)
> > <https://reviews.apache.org/r/64661/diff/5/?file=2073589#file2073589line144>
> >
> > I don't see such new function is added in this interface. Have you updated your code accordingly before you mark my comment to be resolved?
New diff will be uploaded soon. I've addressed it in that patch.
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207474
-----------------------------------------------------------
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207474
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
Lines 144 (patched)
<https://reviews.apache.org/r/64661/#comment290872>
I don't see such new function is added in this interface. Have you updated your code accordingly before you mark my comment to be resolved?
- Na Li
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Aug. 16, 2018, 8:26 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
> > Line 588 (original), 588 (patched)
> > <https://reviews.apache.org/r/64661/diff/5/?file=2073587#file2073587line588>
> >
> > can you change the API to return Set<TSentryRole>? like what's in the following code? It is even simplier.
> >
> > "roleSet = sentryStore.getTSentryRolesByGroupName(groups, checkAllGroups);"
> >
> > https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java#L646
>
> Arjun Mishra wrote:
> Lina, there are two types of TSentryRole, org.apache.sentry.api.generic.thrift.TSentryRole, and org.apache.sentry.api.service.thrift.TSentryRole
1) DelegateSentryStore.getRolesByGroups gets org.apache.sentry.api.service.thrift.TSentryRole right now, then return list of role names
2) You can add a new function in DelegateSentryStore.getRolesByGroups to get org.apache.sentry.api.service.thrift.TSentryRole, and then return set of org.apache.sentry.api.generic.thrift.TSentryRole, which can be returned by SentryGenericPolicyProcessor.list_sentry_roles_by_group
org.apache.sentry.api.service.thrift.TSentryRole is defined in sentry_policy_service.thrift
struct TSentryRole {
1: required string roleName,
2: required set<TSentryGroup> groups,
3: required string grantorPrincipal #Deprecated
}
org.apache.sentry.api.generic.thrift.TSentryRole is defined in sentry_generic_policy_service.thrift
struct TSentryRole {
1: required string roleName,
2: required set<string> groups
}
as you can see, you can easily convert org.apache.sentry.api.service.thrift.TSentryRole to org.apache.sentry.api.generic.thrift.TSentryRole
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207431
-----------------------------------------------------------
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 16, 2018, 8:26 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
> > Line 588 (original), 588 (patched)
> > <https://reviews.apache.org/r/64661/diff/5/?file=2073587#file2073587line588>
> >
> > can you change the API to return Set<TSentryRole>? like what's in the following code? It is even simplier.
> >
> > "roleSet = sentryStore.getTSentryRolesByGroupName(groups, checkAllGroups);"
> >
> > https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java#L646
Lina, there are two types of TSentryRole, org.apache.sentry.api.generic.thrift.TSentryRole, and org.apache.sentry.api.service.thrift.TSentryRole
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207431
-----------------------------------------------------------
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 16, 2018, 8:26 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
> > Line 588 (original), 588 (patched)
> > <https://reviews.apache.org/r/64661/diff/5/?file=2073587#file2073587line588>
> >
> > can you change the API to return Set<TSentryRole>? like what's in the following code? It is even simplier.
> >
> > "roleSet = sentryStore.getTSentryRolesByGroupName(groups, checkAllGroups);"
> >
> > https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java#L646
>
> Arjun Mishra wrote:
> Lina, there are two types of TSentryRole, org.apache.sentry.api.generic.thrift.TSentryRole, and org.apache.sentry.api.service.thrift.TSentryRole
>
> Na Li wrote:
> 1) DelegateSentryStore.getRolesByGroups gets org.apache.sentry.api.service.thrift.TSentryRole right now, then return list of role names
> 2) You can add a new function in DelegateSentryStore.getRolesByGroups to get org.apache.sentry.api.service.thrift.TSentryRole, and then return set of org.apache.sentry.api.generic.thrift.TSentryRole, which can be returned by SentryGenericPolicyProcessor.list_sentry_roles_by_group
>
> org.apache.sentry.api.service.thrift.TSentryRole is defined in sentry_policy_service.thrift
>
> struct TSentryRole {
> 1: required string roleName,
> 2: required set<TSentryGroup> groups,
> 3: required string grantorPrincipal #Deprecated
> }
>
> org.apache.sentry.api.generic.thrift.TSentryRole is defined in sentry_generic_policy_service.thrift
> struct TSentryRole {
> 1: required string roleName,
> 2: required set<string> groups
> }
>
> as you can see, you can easily convert org.apache.sentry.api.service.thrift.TSentryRole to org.apache.sentry.api.generic.thrift.TSentryRole
I ended up doing that
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207431
-----------------------------------------------------------
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207431
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
Line 588 (original), 588 (patched)
<https://reviews.apache.org/r/64661/#comment290826>
can you change the API to return Set<TSentryRole>? like what's in the following code? It is even simplier.
"roleSet = sentryStore.getTSentryRolesByGroupName(groups, checkAllGroups);"
https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java#L646
- Na Li
On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 7:55 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/5/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207504
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Line 243 (original), 265 (patched)
<https://reviews.apache.org/r/64661/#comment290908>
should the comment to changed to "In all calls roles contain exactly one role"?
- Na Li
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207507
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 251 (patched)
<https://reviews.apache.org/r/64661/#comment290912>
If you look at implemenation of "public Set<TSentryRole> getTSentryRolesByGroupName" in SentryStore, it converts list of MSentryRole to list of "org.apache.sentry.api.service.thrift.TSentryRole".
Then in this function, you convert each one to " org.apache.sentry.api.generic.thrift.TSentryRole".
For 5000 roles, for example, it is not trivil.
To get best performance, you can just get list of MSentryRole, and convert them directly to " org.apache.sentry.api.generic.thrift.TSentryRole"
- Na Li
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207532
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 7:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/10/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207566
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On Aug. 17, 2018, 9:47 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 9:47 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b32e7b2f
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/11/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207563
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 335 (original), 335 (patched)
<https://reviews.apache.org/r/64661/#comment290961>
If this is made public, it should be added in SentryStoreInterface.
- Na Li
On Aug. 17, 2018, 9:47 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 9:47 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b32e7b2f
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/11/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207562
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Line 56 (original), 59 (patched)
<https://reviews.apache.org/r/64661/#comment290960>
Should the type be SentryStoreInterface instead of SentryStore? You can look at how SentryService.getSentryStore()
This can be done in another jira. Can you creat a jira to track this?
- Na Li
On Aug. 17, 2018, 9:47 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 9:47 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b32e7b2f
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/11/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207564
-----------------------------------------------------------
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
Line 106 (original), 107 (patched)
<https://reviews.apache.org/r/64661/#comment290962>
can you add test case for getRolesByGroups() in TestDelegateSentryStore to check it handles the null case, and calls getallroles?
- Na Li
On Aug. 17, 2018, 9:47 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 9:47 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b32e7b2f
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/11/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 9:47 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
Changes
-------
Added handling null group
Bugs: SENTRY-1944
https://issues.apache.org/jira/browse/SENTRY-1944
Repository: sentry
Description
-------
When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
It may be possible to optimize it.
Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
Attach one or more files to this issue
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b32e7b2f
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
Diff: https://reviews.apache.org/r/64661/diff/11/
Changes: https://reviews.apache.org/r/64661/diff/10-11/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 8:18 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 255 (patched)
> > <https://reviews.apache.org/r/64661/diff/10/?file=2074734#file2074734line256>
> >
> > 1) I believe that when kafka calls
> >
> > the groups is a list with only one member, which is "null".
> >
> > 1.1) the group name is null in SentryGenericServiceClientDefaultImpl
> > @Override
> > public Set<TSentryRole> listAllRoles(String requestorUserName, String component)
> > throws SentryUserException {
> > return listRolesByGroupName(requestorUserName, null, component);
> > }
> >
> > 1.2) the group name of "null" is in request to generic service
> >
> > public Set<TSentryRole> listRolesByGroupName(
> > String requestorUserName,
> > String groupName,
> > String component)
> > throws SentryUserException {
> > TListSentryRolesRequest request = new TListSentryRolesRequest();
> > request.setProtocol_version(sentry_common_serviceConstants.TSENTRY_SERVICE_V2);
> > request.setRequestorUserName(requestorUserName);
> > request.setGroupName(groupName);
> > request.setComponent(component);
> > TListSentryRolesResponse response;
> > try {
> > response = client.list_sentry_roles_by_group(request);
> > Status.throwIfNotOk(response.getStatus());
> > return response.getRoles();
> > } catch (TException e) {
> > throw new SentryUserException(THRIFT_EXCEPTION_MESSAGE, e);
> > }
> > }
> >
> > 1.3) groups contains "null" when calling DelegateSentryStore.getRolesByGroups().
> >
> > public TListSentryRolesResponse list_sentry_roles_by_group(
> > final TListSentryRolesRequest request) throws TException {
> > Response<Set<TSentryRole>> respose = requestHandle(new RequestHandler<Set<TSentryRole>>() {
> > @Override
> > public Response<Set<TSentryRole>> handle() throws Exception {
> > validateClientVersion(request.getProtocol_version());
> > Set<String> groups = getRequestorGroups(conf, request.getRequestorUserName());
> > if (!AccessConstants.ALL.equalsIgnoreCase(request.getGroupName())) {
> > boolean admin = inAdminGroups(groups);
> > //Only admin users can list all roles in the system ( groupname = null)
> > //Non admin users are only allowed to list only groups which they belong to
> > if(!admin && (request.getGroupName() == null || !groups.contains(request.getGroupName()))) {
> > throw new SentryAccessDeniedException(ACCESS_DENIAL_MESSAGE + request.getRequestorUserName());
> > }
> > groups.clear();
> > groups.add(request.getGroupName());
> > }
> > Set<String> roleNames = store.getRolesByGroups(request.getComponent(), groups);
> >
> >
> > 2) This implementation using SentryStore.getRolesForGroups() does not have groups contains a single element of "null" gracefully. That is why I am really worried about the test you have done for this update.
> >
> > public Set<MSentryRole> getRolesForGroups(PersistenceManager pm, Set<String> groups) {
> > Set<MSentryRole> result = Sets.newHashSet();
> > if (groups != null) {
> > Query query = pm.newQuery(MSentryGroup.class);
> > query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> > query.setFilter(":p1.contains(this.groupName)");
> > List<MSentryGroup> sentryGroups = (List) query.execute(groups.toArray());
> > if (sentryGroups != null) {
> > for (MSentryGroup sentryGroup : sentryGroups) {
> > result.addAll(sentryGroup.getRoles());
> > }
> > }
> > }
> > return result;
> > }
> >
> > 3) You should check if there is a member of null in groups and handle it properly, and have testing case for that scenario.
>
> Arjun Mishra wrote:
> Still going through your comments. But I did add the null check under getRolesByGroups method. See below:
>
> if(groups.contains(null)) {
> true)) {
> roles = delegate.getAllRoleNames();
> roles.add(tSentryRole.getRoleName());
> } else {
> roles = delegate.getRoleNamesForGroups(groups);
> }
> }
Good point. I included the null check in both new methods. Thanks
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207538
-----------------------------------------------------------
On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 7:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/10/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 8:18 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 255 (patched)
> > <https://reviews.apache.org/r/64661/diff/10/?file=2074734#file2074734line256>
> >
> > 1) I believe that when kafka calls
> >
> > the groups is a list with only one member, which is "null".
> >
> > 1.1) the group name is null in SentryGenericServiceClientDefaultImpl
> > @Override
> > public Set<TSentryRole> listAllRoles(String requestorUserName, String component)
> > throws SentryUserException {
> > return listRolesByGroupName(requestorUserName, null, component);
> > }
> >
> > 1.2) the group name of "null" is in request to generic service
> >
> > public Set<TSentryRole> listRolesByGroupName(
> > String requestorUserName,
> > String groupName,
> > String component)
> > throws SentryUserException {
> > TListSentryRolesRequest request = new TListSentryRolesRequest();
> > request.setProtocol_version(sentry_common_serviceConstants.TSENTRY_SERVICE_V2);
> > request.setRequestorUserName(requestorUserName);
> > request.setGroupName(groupName);
> > request.setComponent(component);
> > TListSentryRolesResponse response;
> > try {
> > response = client.list_sentry_roles_by_group(request);
> > Status.throwIfNotOk(response.getStatus());
> > return response.getRoles();
> > } catch (TException e) {
> > throw new SentryUserException(THRIFT_EXCEPTION_MESSAGE, e);
> > }
> > }
> >
> > 1.3) groups contains "null" when calling DelegateSentryStore.getRolesByGroups().
> >
> > public TListSentryRolesResponse list_sentry_roles_by_group(
> > final TListSentryRolesRequest request) throws TException {
> > Response<Set<TSentryRole>> respose = requestHandle(new RequestHandler<Set<TSentryRole>>() {
> > @Override
> > public Response<Set<TSentryRole>> handle() throws Exception {
> > validateClientVersion(request.getProtocol_version());
> > Set<String> groups = getRequestorGroups(conf, request.getRequestorUserName());
> > if (!AccessConstants.ALL.equalsIgnoreCase(request.getGroupName())) {
> > boolean admin = inAdminGroups(groups);
> > //Only admin users can list all roles in the system ( groupname = null)
> > //Non admin users are only allowed to list only groups which they belong to
> > if(!admin && (request.getGroupName() == null || !groups.contains(request.getGroupName()))) {
> > throw new SentryAccessDeniedException(ACCESS_DENIAL_MESSAGE + request.getRequestorUserName());
> > }
> > groups.clear();
> > groups.add(request.getGroupName());
> > }
> > Set<String> roleNames = store.getRolesByGroups(request.getComponent(), groups);
> >
> >
> > 2) This implementation using SentryStore.getRolesForGroups() does not have groups contains a single element of "null" gracefully. That is why I am really worried about the test you have done for this update.
> >
> > public Set<MSentryRole> getRolesForGroups(PersistenceManager pm, Set<String> groups) {
> > Set<MSentryRole> result = Sets.newHashSet();
> > if (groups != null) {
> > Query query = pm.newQuery(MSentryGroup.class);
> > query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> > query.setFilter(":p1.contains(this.groupName)");
> > List<MSentryGroup> sentryGroups = (List) query.execute(groups.toArray());
> > if (sentryGroups != null) {
> > for (MSentryGroup sentryGroup : sentryGroups) {
> > result.addAll(sentryGroup.getRoles());
> > }
> > }
> > }
> > return result;
> > }
> >
> > 3) You should check if there is a member of null in groups and handle it properly, and have testing case for that scenario.
Still going through your comments. But I did add the null check under getRolesByGroups method. See below:
if(groups.contains(null)) {
true)) {
roles = delegate.getAllRoleNames();
roles.add(tSentryRole.getRoleName());
} else {
roles = delegate.getRoleNamesForGroups(groups);
}
}
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207538
-----------------------------------------------------------
On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 7:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/10/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207538
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 255 (patched)
<https://reviews.apache.org/r/64661/#comment290934>
1) I believe that when kafka calls
the groups is a list with only one member, which is "null".
1.1) the group name is null in SentryGenericServiceClientDefaultImpl
@Override
public Set<TSentryRole> listAllRoles(String requestorUserName, String component)
throws SentryUserException {
return listRolesByGroupName(requestorUserName, null, component);
}
1.2) the group name of "null" is in request to generic service
public Set<TSentryRole> listRolesByGroupName(
String requestorUserName,
String groupName,
String component)
throws SentryUserException {
TListSentryRolesRequest request = new TListSentryRolesRequest();
request.setProtocol_version(sentry_common_serviceConstants.TSENTRY_SERVICE_V2);
request.setRequestorUserName(requestorUserName);
request.setGroupName(groupName);
request.setComponent(component);
TListSentryRolesResponse response;
try {
response = client.list_sentry_roles_by_group(request);
Status.throwIfNotOk(response.getStatus());
return response.getRoles();
} catch (TException e) {
throw new SentryUserException(THRIFT_EXCEPTION_MESSAGE, e);
}
}
1.3) groups contains "null" when calling DelegateSentryStore.getRolesByGroups().
public TListSentryRolesResponse list_sentry_roles_by_group(
final TListSentryRolesRequest request) throws TException {
Response<Set<TSentryRole>> respose = requestHandle(new RequestHandler<Set<TSentryRole>>() {
@Override
public Response<Set<TSentryRole>> handle() throws Exception {
validateClientVersion(request.getProtocol_version());
Set<String> groups = getRequestorGroups(conf, request.getRequestorUserName());
if (!AccessConstants.ALL.equalsIgnoreCase(request.getGroupName())) {
boolean admin = inAdminGroups(groups);
//Only admin users can list all roles in the system ( groupname = null)
//Non admin users are only allowed to list only groups which they belong to
if(!admin && (request.getGroupName() == null || !groups.contains(request.getGroupName()))) {
throw new SentryAccessDeniedException(ACCESS_DENIAL_MESSAGE + request.getRequestorUserName());
}
groups.clear();
groups.add(request.getGroupName());
}
Set<String> roleNames = store.getRolesByGroups(request.getComponent(), groups);
2) This implementation using SentryStore.getRolesForGroups() does not have groups contains a single element of "null" gracefully. That is why I am really worried about the test you have done for this update.
public Set<MSentryRole> getRolesForGroups(PersistenceManager pm, Set<String> groups) {
Set<MSentryRole> result = Sets.newHashSet();
if (groups != null) {
Query query = pm.newQuery(MSentryGroup.class);
query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
query.setFilter(":p1.contains(this.groupName)");
List<MSentryGroup> sentryGroups = (List) query.execute(groups.toArray());
if (sentryGroups != null) {
for (MSentryGroup sentryGroup : sentryGroups) {
result.addAll(sentryGroup.getRoles());
}
}
}
return result;
}
3) You should check if there is a member of null in groups and handle it properly, and have testing case for that scenario.
- Na Li
On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 7:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/10/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207544
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 255 (patched)
<https://reviews.apache.org/r/64661/#comment290940>
You add that check in function "getRolesByGroups()". I am talking about the function "getTSentryRolesByGroupName()". They are not the same function.
- Na Li
On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 7:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/10/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 7:01 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
Changes
-------
Post feedback
Bugs: SENTRY-1944
https://issues.apache.org/jira/browse/SENTRY-1944
Repository: sentry
Description
-------
When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
It may be possible to optimize it.
Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
Attach one or more files to this issue
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
Diff: https://reviews.apache.org/r/64661/diff/10/
Changes: https://reviews.apache.org/r/64661/diff/9-10/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 5:12 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
> > Lines 135 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074618#file2074618line135>
> >
> > we should have a test for this new function.
> >
> > Do we have e2e test for generic service list_sentry_roles_by_group()
e2e tests will be covered by e2e tests for Kafla, Solr and Sqoop
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207513
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207513
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
Lines 135 (patched)
<https://reviews.apache.org/r/64661/#comment290920>
we should have a test for this new function.
Do we have e2e test for generic service list_sentry_roles_by_group()
- Na Li
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 244 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line244>
> >
> > We should do something to avoid this conflict with the TSentryRole type. getTSentryRolesByGroupName() is the implementation of the SentryStoreLayer, but this returns a different type which breaks the contract of the implementation.
> >
> > Why is the SentryStoreLayer using a different TSentryRole than the DelegationSentryStore? If we use api.service.thrift.TSentryRole in SentryStoreLayer, then that would fix the problem.
>
> Arjun Mishra wrote:
> It doesn't return a different type. They both return org.apache.sentry.api.generic.thrift.TSentryRole
>
> Arjun Mishra wrote:
> DelegateSentryStore calls SentryStore.getTSentryRolesByGroupName() which uses org.apache.sentry.api.service.thrift.TSentryRole.
>
> Na Li wrote:
> I agree with Arjun.
> DelegationSentryStore.getTSentryRolesByGroupName() returns set of org.apache.sentry.api.generic.thrift.TSentryRole
> SentryStore, which is called by DelegationSentryStore, returns set of org.apache.sentry.api.service.thrift.TSentryRole.
> That is why DelegationSentryStore calls SentryStore and converts org.apache.sentry.api.service.thrift.TSentryRole to org.apache.sentry.api.generic.thrift.TSentryRole
Isn't this a performance issue as well? The SentryStore reads a set of MSentryRole which is converted to service.thrift.TSentryRole which is then converted to generic.thrift.TSentryRole. That does not look right.
Should we have another SentryStore method that returns the set of MSentryRole instead and just walk through it to convert it to generic.TSentryRole?
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
> > Lines 23 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074618#file2074618line23>
> >
> > If this uses the service.thrift then the DelegationSentrySTore won't have the issue with the types.
DelegateSentryStore cannot use service.thrift because it is called by SentryGenericPolicyProcessor, as opposed to SentryPolicyStoreProcessor.
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 244 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line244>
> >
> > We should do something to avoid this conflict with the TSentryRole type. getTSentryRolesByGroupName() is the implementation of the SentryStoreLayer, but this returns a different type which breaks the contract of the implementation.
> >
> > Why is the SentryStoreLayer using a different TSentryRole than the DelegationSentryStore? If we use api.service.thrift.TSentryRole in SentryStoreLayer, then that would fix the problem.
>
> Arjun Mishra wrote:
> It doesn't return a different type. They both return org.apache.sentry.api.generic.thrift.TSentryRole
DelegateSentryStore calls SentryStore.getTSentryRolesByGroupName() which uses org.apache.sentry.api.service.thrift.TSentryRole.
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 266 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line266>
> >
> > This will throw a NullPointerException if roles is null. Is it necessary to throw an exception? Can we return an emptySet instead?
Good point. We can return empty set. No need to throw NPE
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 244 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line244>
> >
> > We should do something to avoid this conflict with the TSentryRole type. getTSentryRolesByGroupName() is the implementation of the SentryStoreLayer, but this returns a different type which breaks the contract of the implementation.
> >
> > Why is the SentryStoreLayer using a different TSentryRole than the DelegationSentryStore? If we use api.service.thrift.TSentryRole in SentryStoreLayer, then that would fix the problem.
>
> Arjun Mishra wrote:
> It doesn't return a different type. They both return org.apache.sentry.api.generic.thrift.TSentryRole
>
> Arjun Mishra wrote:
> DelegateSentryStore calls SentryStore.getTSentryRolesByGroupName() which uses org.apache.sentry.api.service.thrift.TSentryRole.
I agree with Arjun.
DelegationSentryStore.getTSentryRolesByGroupName() returns set of org.apache.sentry.api.generic.thrift.TSentryRole
SentryStore, which is called by DelegationSentryStore, returns set of org.apache.sentry.api.service.thrift.TSentryRole.
That is why DelegationSentryStore calls SentryStore and converts org.apache.sentry.api.service.thrift.TSentryRole to org.apache.sentry.api.generic.thrift.TSentryRole
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 250-258 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line250>
> >
> > This method returns a set of TSentryRole, isn't the getTSentryRolesByGroupName() call enough? Why is it neede to walk through the Set returned by the call, and create another set of TSentryRole?
The other call is from SentryStore which returns a different type ot TSentryRole
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 244 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line244>
> >
> > We should do something to avoid this conflict with the TSentryRole type. getTSentryRolesByGroupName() is the implementation of the SentryStoreLayer, but this returns a different type which breaks the contract of the implementation.
> >
> > Why is the SentryStoreLayer using a different TSentryRole than the DelegationSentryStore? If we use api.service.thrift.TSentryRole in SentryStoreLayer, then that would fix the problem.
>
> Arjun Mishra wrote:
> It doesn't return a different type. They both return org.apache.sentry.api.generic.thrift.TSentryRole
>
> Arjun Mishra wrote:
> DelegateSentryStore calls SentryStore.getTSentryRolesByGroupName() which uses org.apache.sentry.api.service.thrift.TSentryRole.
>
> Na Li wrote:
> I agree with Arjun.
> DelegationSentryStore.getTSentryRolesByGroupName() returns set of org.apache.sentry.api.generic.thrift.TSentryRole
> SentryStore, which is called by DelegationSentryStore, returns set of org.apache.sentry.api.service.thrift.TSentryRole.
> That is why DelegationSentryStore calls SentryStore and converts org.apache.sentry.api.service.thrift.TSentryRole to org.apache.sentry.api.generic.thrift.TSentryRole
>
> Sergio Pena wrote:
> Isn't this a performance issue as well? The SentryStore reads a set of MSentryRole which is converted to service.thrift.TSentryRole which is then converted to generic.thrift.TSentryRole. That does not look right.
>
> Should we have another SentryStore method that returns the set of MSentryRole instead and just walk through it to convert it to generic.TSentryRole?
I am just about to update that. Lina suggested the same. Now it will convert directly to generic.TSentryRole
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 3:01 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 244 (patched)
> > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line244>
> >
> > We should do something to avoid this conflict with the TSentryRole type. getTSentryRolesByGroupName() is the implementation of the SentryStoreLayer, but this returns a different type which breaks the contract of the implementation.
> >
> > Why is the SentryStoreLayer using a different TSentryRole than the DelegationSentryStore? If we use api.service.thrift.TSentryRole in SentryStoreLayer, then that would fix the problem.
It doesn't return a different type. They both return org.apache.sentry.api.generic.thrift.TSentryRole
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207496
-----------------------------------------------------------
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/#review207496
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 244 (patched)
<https://reviews.apache.org/r/64661/#comment290900>
We should do something to avoid this conflict with the TSentryRole type. getTSentryRolesByGroupName() is the implementation of the SentryStoreLayer, but this returns a different type which breaks the contract of the implementation.
Why is the SentryStoreLayer using a different TSentryRole than the DelegationSentryStore? If we use api.service.thrift.TSentryRole in SentryStoreLayer, then that would fix the problem.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 250-258 (patched)
<https://reviews.apache.org/r/64661/#comment290901>
This method returns a set of TSentryRole, isn't the getTSentryRolesByGroupName() call enough? Why is it neede to walk through the Set returned by the call, and create another set of TSentryRole?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Lines 266 (patched)
<https://reviews.apache.org/r/64661/#comment290902>
This will throw a NullPointerException if roles is null. Is it necessary to throw an exception? Can we return an emptySet instead?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
Lines 23 (patched)
<https://reviews.apache.org/r/64661/#comment290903>
If this uses the service.thrift then the DelegationSentrySTore won't have the issue with the types.
- Sergio Pena
On Aug. 17, 2018, 2:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 2:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/9/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 2:01 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
Bugs: SENTRY-1944
https://issues.apache.org/jira/browse/SENTRY-1944
Repository: sentry
Description
-------
When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
It may be possible to optimize it.
Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
Attach one or more files to this issue
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
Diff: https://reviews.apache.org/r/64661/diff/9/
Changes: https://reviews.apache.org/r/64661/diff/8-9/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 1:08 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
Bugs: SENTRY-1944
https://issues.apache.org/jira/browse/SENTRY-1944
Repository: sentry
Description
-------
When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
It may be possible to optimize it.
Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
Attach one or more files to this issue
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
Diff: https://reviews.apache.org/r/64661/diff/8/
Changes: https://reviews.apache.org/r/64661/diff/7-8/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 1:07 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
Bugs: SENTRY-1944
https://issues.apache.org/jira/browse/SENTRY-1944
Repository: sentry
Description
-------
When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
It may be possible to optimize it.
Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
Attach one or more files to this issue
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
Diff: https://reviews.apache.org/r/64661/diff/7/
Changes: https://reviews.apache.org/r/64661/diff/6-7/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64661: SENTRY-1944: Optimize
DelegateSentryStore.getGroupsByRoles() and update
SentryGenericPolicyProcessor to retrieve roles to group mapping in a single
transaction
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/64661/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 1:03 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and Sergio Pena.
Changes
-------
Post feedback
Bugs: SENTRY-1944
https://issues.apache.org/jira/browse/SENTRY-1944
Repository: sentry
Description
-------
When Solr is using Sentry server for authorization, it issues a lot of calls to getGroupsByRoles() function in DelegateSentryStore.
This function isn't very efficient - it walks over each role in the set, obtains role by name, get groups for each role, and collects all group names into a set.
It may be possible to optimize it.
Also, in SentryGenericPolicyProcessor class method list_sentry_roles_by_group() would make N transactions to build the roles to set of groups map. Instead, make it to a single transaction. This will significantly speed up operation
Attach one or more files to this issue
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java 1cc4b1b37
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 3026a6225
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java eec2757d3
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java 4c207e9b4
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java 69d16238f
Diff: https://reviews.apache.org/r/64661/diff/6/
Changes: https://reviews.apache.org/r/64661/diff/5-6/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra