----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64661/#review207430 -----------------------------------------------------------
What's the expected performance improvement we're getting with this? Have you run tests to check how much time we're saving? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java Line 590 (original), 590 (patched) <https://reviews.apache.org/r/64661/#comment290820> Code convention: Need a space to separate the variable name from the type. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java Lines 244-246 (original), 249-252 (patched) <https://reviews.apache.org/r/64661/#comment290819> Is it necessary to throw a NullPointerException if roles is null? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java Lines 278 (patched) <https://reviews.apache.org/r/64661/#comment290821> Is it necessary to throw a NPE if roles is null? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java Lines 294 (patched) <https://reviews.apache.org/r/64661/#comment290822> You can construct the hasmap with an initial capacitity of roles.size() to avoid the hashmap to be resized during the for loop. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java Lines 295-297 (patched) <https://reviews.apache.org/r/64661/#comment290823> Code convention: Space between for and () Btw, this has 2 loops. I think we can avoid using a 2nd loop by making the query to request all roles instead of all groups. That way you just walk through the list of roles and put each role and its groups in the map (this saves one loop and the containsKey and get calls). sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java Lines 139-142 (patched) <https://reviews.apache.org/r/64661/#comment290824> Need to fill information of @param, @return and @throws. I usually put in @throws why I am expecting this exception. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java Lines 292 (patched) <https://reviews.apache.org/r/64661/#comment290825> Coding convention: space >mockMap. - Sergio Pena On Aug. 16, 2018, 7:55 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64661/ > ----------------------------------------------------------- > > (Updated Aug. 16, 2018, 7:55 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/5/ > > > Testing > ------- > > mvn -f sentry-provider/sentry-provider-db/pom.xml test > > > Thanks, > > Arjun Mishra > >
