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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 328 (patched)
<https://reviews.apache.org/r/61863/#comment291189>

    what is the purpose of adding this?
    
    IN package.jdo,
    
    <class name="MSentryRole" identity-type="datastore" table="SENTRY_ROLE" 
detachable="true">
    
    You can see the privileges and other fields are all in default fetching 
group
    
    "<field name = "privileges" table="SENTRY_ROLE_DB_PRIVILEGE_MAP" 
default-fetch-group="true">"
    
    So I don't think the code change here will make a difference. Have you 
tested that this change does improve performance?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 365 (patched)
<https://reviews.apache.org/r/61863/#comment291190>

    privileges is in default fetch group of MSentryUser
    
    <class name="MSentryUser" identity-type="datastore" table="SENTRY_USER" 
detachable="true">
          <datastore-identity>
            <column name="USER_ID"/>
          </datastore-identity>
          <field name="userName">
            <column name="USER_NAME" length="128" jdbc-type="VARCHAR"/>
            <index name="SentryUserName" unique="true"/>
          </field>
          <field name = "createTime">
            <column name = "CREATE_TIME" jdbc-type="BIGINT"/>
          </field>
    
          <field name="roles" mapped-by="users">
             <collection 
element-type="org.apache.sentry.provider.db.service.model.MSentryRole"/>
          </field>
    
          <field name = "privileges" table="SENTRY_USER_DB_PRIVILEGE_MAP" 
default-fetch-group="true">



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 1466 (patched)
<https://reviews.apache.org/r/61863/#comment291191>

    This will make roles fetched for privileges automatically



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2239 (original), 2280 (patched)
<https://reviews.apache.org/r/61863/#comment291195>

    can you reduce overhead by
    
    result.add(convertToTsentryRole( role, mgroup.getGroupName))? 
    
      private TSentryRole convertToTSentryRole(MSentryRole mSentryRole, String 
groupName) {
        String roleName = mSentryRole.getRoleName().intern();
        
        Set<TSentryGroup> sentryGroups = new HashSet<>(1);
        sentryGroups.add(new TSentryGroup(groupName.intern());
         
        
    
        return new TSentryRole(roleName, sentryGroups, EMPTY_GRANTOR_PRINCIPAL);
      }
      
      In this way, you don't need to need to mSentryRole.getGroups(), which 
should cause another call to DB to get the groups of a role.
      
      In this way, we only have DB access to get a group and its roles. No 
fetching of privileges and other groups of each role that is associated with 
the specific group


- Na Li


On Aug. 21, 2018, 7:19 p.m., Arjun Mishra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2018, 7:19 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -----
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0ef6a208f 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Reply via email to