subject:"Re\: Review Request 64661\: SENTRY\-1944\: Optimize DelegateSentryStore.getGroupsByRoles\(\) and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction"

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

2018-08-17 Thread Na Li via Review Board


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

2018-08-17 Thread Na Li via Review Board


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


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

2018-08-17 Thread Na Li via Review Board


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


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

2018-08-17 Thread Na Li via Review Board


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


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

2018-08-17 Thread Arjun Mishra via Review Board


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

2018-08-17 Thread Arjun Mishra via Review Board



> 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)
> > 
> >
> > 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 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 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> respose = requestHandle(new 
> > RequestHandler>() {
> >   @Override
> >   public Response> handle() throws Exception {
> > validateClientVersion(request.getProtocol_version());
> > Set 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 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 getRolesForGroups(PersistenceManager pm, 
> > Set groups) {
> > Set 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 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

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

2018-08-17 Thread Na Li via Review Board


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


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

2018-08-17 Thread Arjun Mishra via Review Board



> 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)
> > 
> >
> > 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 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 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> respose = requestHandle(new 
> > RequestHandler>() {
> >   @Override
> >   public Response> handle() throws Exception {
> > validateClientVersion(request.getProtocol_version());
> > Set 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 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 getRolesForGroups(PersistenceManager pm, 
> > Set groups) {
> > Set 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 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/
> --

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

2018-08-17 Thread Na Li via Review Board


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


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 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 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> respose = requestHandle(new 
RequestHandler>() {
  @Override
  public Response> handle() throws Exception {
validateClientVersion(request.getProtocol_version());
Set 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 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 getRolesForGroups(PersistenceManager pm, 
Set groups) {
Set 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 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 
>

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

2018-08-17 Thread Sergio Pena via Review Board


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

2018-08-17 Thread Arjun Mishra via Review Board


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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-17 Thread Sergio Pena via Review Board



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

2018-08-17 Thread Na Li via Review Board


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


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

2018-08-17 Thread Na Li via Review Board


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


If you look at implemenation of "public Set 
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

2018-08-17 Thread Na Li via Review Board



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

2018-08-17 Thread Na Li via Review Board


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


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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-17 Thread Sergio Pena via Review Board


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


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)


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)


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)


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

2018-08-17 Thread Arjun Mishra via Review Board


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

2018-08-17 Thread Arjun Mishra via Review Board


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

2018-08-17 Thread Arjun Mishra via Review Board


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

2018-08-17 Thread Arjun Mishra via Review Board


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

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction

2018-08-17 Thread Arjun Mishra via Review Board



> 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)
> > 
> >
> > can you change the API to return Set? 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 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 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

2018-08-17 Thread Arjun Mishra via Review Board



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

2018-08-16 Thread Na Li via Review Board


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


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

2018-08-16 Thread Na Li via Review Board



> 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

2018-08-16 Thread Na Li via Review Board



> 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)
> > 
> >
> > can you change the API to return Set? 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 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 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

2018-08-16 Thread Arjun Mishra via Review Board



> 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)
> > 
> >
> > can you change the API to return Set? 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

2018-08-16 Thread Arjun Mishra via Review Board



> 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

2018-08-16 Thread Na Li via Review Board


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


I would rather add the function 

Set getRolesByGroups(String component, Set 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

2018-08-16 Thread Na Li via Review Board


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


can you change the API to return Set? 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

2018-08-16 Thread Sergio Pena via Review Board


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


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)


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)


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)


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)


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)


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)


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

2018-08-16 Thread Arjun Mishra via Review Board


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

39 matches

Mail list logo