----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26678/#review56759 -----------------------------------------------------------
Great first crack at this! I have a couple of NITs below and a few design thoughts: 1. Will this be a Hadoop2 only mechanism? Or is there a follow up Jira for Hadoop1? 2. The AuthenticationManager seems to be an all-in-one singleton. In practice, this may work fine, but I'm concerned adding other forms of auth. might be difficult. Does it make any sense to add some abstractions to take care of the different authentication mechanisms and make it pluggable? core/src/main/java/org/apache/sqoop/authentication/AuthenticationConstants.java <https://reviews.apache.org/r/26678/#comment97152> NIT: Separate constant for 'kerberos' prefix? core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment97153> Missing Java Doc. Also, should we have an Abstract Class to make AuthenticationManager's pluggable? It seems like this one is intended to be for kerberos mainly. core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment97163> Does it make sense to delegate this information to some other abstraction since we might not even use it? core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment97156> Not sure if we should authenticate upon initialization of AuthenticationManager. How about making secureLogin public? core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment97155> Make this public and explicitly call? core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment97158> How about a factory instead? For example, if not Kerberos or simple, maybe LDAP will be used instead? - Abraham Elmahrek On Oct. 14, 2014, 7:44 a.m., richard zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26678/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2014, 7:44 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Kerberos support when starting service > > > Diffs > ----- > > core/pom.xml 2b6e436d637eb03493323e5b36e31e433c1f8bbb > > core/src/main/java/org/apache/sqoop/authentication/AuthenticationConstants.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/authentication/AuthenticationError.java > PRE-CREATION > > core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/core/SqoopServer.java > ac836c7cee010144696ab17645ccd008aed5762d > dist/src/main/server/conf/sqoop.properties > bb010166120321899425f84edb8e1ad6512626d2 > pom.xml f25a29f6db673e6080dcd5ccd51bab76ab38bff4 > > Diff: https://reviews.apache.org/r/26678/diff/ > > > Testing > ------- > > > Thanks, > > richard zhou > >
