> 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) > > <https://reviews.apache.org/r/64661/diff/9/?file=2074617#file2074617line244> > > > > 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 > >
