> On June 16, 2016, 3:01 p.m., Sergio Pena wrote: > > The code looks good for me. > > > > What other tests did you do to validate this code? > > - HS2 with KERBEROS + LDAP? > > - HS2 with KERBEROS only? > > - HS2 with LDAP only?
The unit tests cover the cases: 1. HS2 with kerberos + SASL non-kerberos authentication. I used the CUSTOM authentication since Hive does not have LDAP end-to-end unit test framework and CUSTOM authentication shares the same code path with LDAP except the detail how LDAP does its authentication, which is not the scope of this JIRA. So in term of this JIRA, testing with LDAP and CUSTOM authentication is equalivent and actually LDAP is a specially case of CUSTOM. 2. HS2 with kerberos only 3. HS2 with only SASL non-kerberos authentication (CUSTOM) for same reason as item 1. > On June 16, 2016, 3:01 p.m., Sergio Pena wrote: > > service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java, line 231 > > <https://reviews.apache.org/r/48771/diff/1/?file=1420643#file1420643line231> > > > > Should we return empty strings instead of nulls? That way we avoid that > > any other future developer uses this method without validating a null > > pointer, that could cause NPE if they do not do that. Thanks Sergio for review. yeah, it is debatable which way is better, returning null or empty object (e.g. String), please see http://stackoverflow.com/questions/1626597/should-functions-return-null-or-an-empty-object Thinking over, I tend to still return null for this API because 1. There is not an "empty" authentication mechanism. It either exists like TOKEN, KERBEROS, or does not exist (which null will be more informative) 2. Be consistent with other APIs like getRemoteUser(), getIpAddress() I wonder if it makes sense. Thanks - Chaoyu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48771/#review137988 ----------------------------------------------------------- On June 16, 2016, 2:33 a.m., Chaoyu Tang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48771/ > ----------------------------------------------------------- > > (Updated June 16, 2016, 2:33 a.m.) > > > Review request for hive. > > > Bugs: HIVE-13590 > https://issues.apache.org/jira/browse/HIVE-13590 > > > Repository: hive-git > > > Description > ------- > > Hive should not use Hadoop security (e.g. kerberos) related APIs such as > KerberosName etc to process user logged in via other SASL mechanism such as > LDAP. > > > Diffs > ----- > > > itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcNonKrbSASLWithMiniKdc.java > 1c1beda > service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java ab8806c > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java > 8bc3d94 > > shims/common/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java > 8a4786c > > Diff: https://reviews.apache.org/r/48771/diff/ > > > Testing > ------- > > Manual test > PreCommit test > > > Thanks, > > Chaoyu Tang > >
