> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2426 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579539#file1579539line2426> > > > > Just curious why we don't just put the constant string > > "hive.server2.authentication.ldap.userMembershipKey" here like most of > > other entries?
Because it uses in several places. In particular in documentation. Putting a string in documentation is not maintainable, because later someone can change the string and forget to update in in all places. Documentation would become stale. In such a big project in will be a problem. JavaDoc has means to prevent that from happening by using string constants in documentation sections. > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, > > line 90 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line90> > > > > This seems to be a useful info that will help in diagnostics. Wondering > > why changes from info to debug level? I totally agree, but Naveen doesn't want to expose group names in logs. It is a questionable concern, but moving it to DEBUG may be a good compromise. > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, > > line 115 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line115> > > > > This should be info level which will be consistent with > > GroupMembershipKeyFilter class. Ok. I'll generate 2 log entries then: 1. INFO without group information; 2. DEBUG with full information. Does it make sense? See Naveen's comments for more details. > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, > > line 124 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line124> > > > > Seems 'warn' is not necessary since that could be expected in the for > > loop, right? It means we have a group in configuration that doesn't exist... Would you recommend log it at DEBUG level? > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, > > line 132 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line132> > > > > Since we are throwing the exception, I guess such debug may be > > redundant. We should display such exception in the caller somewhere. Exception message has a different (less descriptive) message. Please see Naveen's comments for more details. > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, > > line 139 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line139> > > > > Seems this could be a info level message. Same here. > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, > > line 145 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line145> > > > > 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. I will update the message. > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java, > > line 265 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579546#file1579546line265> > > > > You may need to add some tests for the default configuraiton which is > > null for HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY. If HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY is NULL this filter will not be used. I think we have enough test for this case. Did I get you correctly? Could you please provide more details about the test case you have in mind? > On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java, line > > 138 > > <https://reviews.apache.org/r/53204/diff/3/?file=1579544#file1579544line138> > > > > 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. I think it should not happen, but I'll double check. - Illya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53204/#review158532 ----------------------------------------------------------- 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 > >
