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