> On Aug. 21, 2018, 8:34 p.m., Na Li wrote: > > 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/diff/4/?file=2075761#file2075761line328> > > > > 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?
default fetch group only fetches primitive types and String objects but not anything else. I did test performance improvement with show roles and show tables command and they were significantly faster > On Aug. 21, 2018, 8:34 p.m., Na Li wrote: > > 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/diff/4/?file=2075761#file2075761line365> > > > > 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"> That means the attributes of privileges will be fetched in default-fetch-group, but not the privilege object when fetching sentry_users > On Aug. 21, 2018, 8:34 p.m., Na Li wrote: > > 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/diff/4/?file=2075761#file2075761line1466> > > > > This will make roles fetched for privileges automatically It should make privilege fetch for roles and users > On Aug. 21, 2018, 8:34 p.m., Na Li wrote: > > 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/diff/4/?file=2075761#file2075761line2280> > > > > 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 I wasn't sure if all groups needed to be fetched for that role. I think I could do that in a different ticket. Let me know if you agree - Arjun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61863/#review207690 ----------------------------------------------------------- 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 > >
