> On June 27, 2018, 6:15 p.m., Arjun Mishra wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 4729 (patched) > > <https://reviews.apache.org/r/67759/diff/2/?file=2046411#file2046411line4729> > > > > I think we'll hit performance issues with these two methods. Doing > > mSentryRole.getPrivileges() or mSentryUser.getPrivileges() will be slow. Do > > we know if this fetch is lazy? > > > > I think we should mimic or improve what is in > > getMSentryPrivilegesByAuth() > > Sergio Pena wrote: > The intention of the API is to return a map between name -> > set<privileges>. That means that I should request all roles or users of the > system, and get the privileges assigned to them and put them in a map. > If I do mimic getMSentryPrivilegesByAuth, then that will return all > privileges -> map mixed with roles and users, and then I will need to walk > through all of them to map name -> set<privileges>. > > Arjun Mishra wrote: > But you pass in EntityType right. WOuld you still get privileges for all > roles and users? > > Sergio Pena wrote: > getMSentryPrivilegesByAuth() returns a set of privileges only, not a map. > That means that if a request only by EntityType.ROLE, then it will return all > privileges for all roles, but then I will have to map to role to a privilege. > That will require O(n) to walk through the set of privileges and then > separate them into a map roleName -> privileges. > > Btw, I added the fetchGroup to avoid lazy fetching and hit a performance > issue. > > Na Li wrote: > Notice that "default-fetch-group="true"" for role, so when getting role > from DB, the privileges will be get as well, not lazy load. > > <class name="MSentryRole" identity-type="datastore" > table="SENTRY_ROLE" detachable="true"> > <datastore-identity> > <column name="ROLE_ID"/> > </datastore-identity> > <field name="roleName"> > <column name="ROLE_NAME" length="128" jdbc-type="VARCHAR"/> > <index name="SentryRoleName" unique="true"/> > </field> > <field name = "createTime"> > <column name = "CREATE_TIME" jdbc-type="BIGINT"/> > </field> > <field name = "privileges" table="SENTRY_ROLE_DB_PRIVILEGE_MAP" > default-fetch-group="true"> > <collection > element-type="org.apache.sentry.provider.db.service.model.MSentryPrivilege"/> > <join> > <column name="ROLE_ID"/> > </join> > <element> > <column name="DB_PRIVILEGE_ID"/> > </element> > </field>
So the fetch group is already enabled, then I don't need the change onthe patch #3. Lina, are you ok on patch #2 then? - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67759/#review205455 ----------------------------------------------------------- On June 27, 2018, 7:33 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67759/ > ----------------------------------------------------------- > > (Updated June 27, 2018, 7:33 p.m.) > > > Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li. > > > Bugs: sentry-2284 > https://issues.apache.org/jira/browse/sentry-2284 > > > Repository: sentry > > > Description > ------- > > Add two API methods to the Sentry client: > - listAllRolesPrivileges > - listAllUsersPrivileges > > Both methods return a map object of the form: > - [roleName, set<privileges>] > - [userName, set<privileges>] > > Unit tests, thrift API code and Client methods are provided. > > > Diffs > ----- > > sentry-dist/src/license/THIRD-PARTY.properties > b39e1b6ca7eba8c6a7695a4238104af7cd50da32 > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java > d456d7ff64a8146473ff14a65f06e7cb664939b7 > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesRequest.java > PRE-CREATION > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java > PRE-CREATION > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java > dc1d67ba16c9c7acf76eb3318864ba0606e2aa5a > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 5f76cb0aa3bd17d6aca4adc52ba59ded5ec0b900 > > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift > 56aedcb5267495e610e3dace1f38e708d68ffe84 > > sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyServiceClientDefaultImpl.java > 1666e326462b771674930e52e3790ca92f467778 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 39271500df179e0ddf8ce4f1589eaaa8137afa25 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > d8ab1fc90255c6ed42c00e3d3aa6103e47a40b29 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java > f61ae575c99474a76886d3c7a74765fdb067acd6 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java > de4e0012d521cf402f4773a04b673f8056a5337c > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 52ce72cebc8b69f08988b05ba54e2bbedd201c1a > > > Diff: https://reviews.apache.org/r/67759/diff/3/ > > > Testing > ------- > > > Thanks, > > Sergio Pena > >
