----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53204/#review158532 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2425) <https://reviews.apache.org/r/53204/#comment229295> Just curious why we don't just put the constant string "hive.server2.authentication.ldap.userMembershipKey" here like most of other entries? service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java (line 90) <https://reviews.apache.org/r/53204/#comment229296> This seems to be a useful info that will help in diagnostics. Wondering why changes from info to debug level? service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java (line 115) <https://reviews.apache.org/r/53204/#comment229297> This should be info level which will be consistent with GroupMembershipKeyFilter class. service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java (line 124) <https://reviews.apache.org/r/53204/#comment229298> Seems 'warn' is not necessary since that could be expected in the for loop, right? service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java (line 132) <https://reviews.apache.org/r/53204/#comment229299> Since we are throwing the exception, I guess such debug may be redundant. We should display such exception in the caller somewhere. service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java (line 139) <https://reviews.apache.org/r/53204/#comment229300> Seems this could be a info level message. service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java (line 145) <https://reviews.apache.org/r/53204/#comment229301> You may need to change message since it's expected that the user is not in some groups. Probably change to "Cannot match user ... and group ..." since "Failed to" seems to be an error. service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java (line 138) <https://reviews.apache.org/r/53204/#comment229302> Looks like we won't handle NPE so NPE may cause some problems. If userMembershipAttr is null, will we still check userMememberOfGroup or not? If not, maybe we should handle such exception here. How about groupMembershipAttr above? Seems we will have such issue as well. service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java (line 265) <https://reviews.apache.org/r/53204/#comment229303> You may need to add some tests for the default configuraiton which is null for HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY. - Aihua Xu On Dec. 8, 2016, 12:45 a.m., Illya Yalovyy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53204/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2016, 12:45 a.m.) > > > Review request for hive, Aihua Xu, Ashutosh Chauhan, Chaoyu Tang, and Szehon > Ho. > > > Repository: hive-git > > > Description > ------- > > HIVE-15076 Improve scalability of LDAP authentication provider group filter > > https://issues.apache.org/jira/browse/HIVE-15076 > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 5ea9751 > service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 33b6088 > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java > 152c4b2 > service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 65076ea > service/src/java/org/apache/hive/service/auth/ldap/Query.java b8bf938 > service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java > e9172d3 > > service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java > cd62935 > > service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java > 4fad755 > > service/src/test/org/apache/hive/service/auth/ldap/LdapAuthenticationTestCase.java > acde8c1 > service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java > 0cc2ead > service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java > 499b624 > service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java > 3054e33 > service/src/test/resources/ldap/ad.example.com.ldif PRE-CREATION > service/src/test/resources/ldap/microsoft.schema.ldif PRE-CREATION > > Diff: https://reviews.apache.org/r/53204/diff/ > > > Testing > ------- > > Build succeeded. > > Test results: > > Tests run: 149, Failures: 0, Errors: 0, Skipped: 0 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 03:14 min > [INFO] Finished at: 2016-10-26T13:53:15-07:00 > [INFO] Final Memory: 36M/1091M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Illya Yalovyy > >
