----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26678/#review57731 -----------------------------------------------------------
Nice. Are there any tests pending? core/src/main/java/org/apache/sqoop/authentication/AuthenticationConstants.java <https://reviews.apache.org/r/26678/#comment98566> is this for the sqoop.properties? is that what it means with sqoop configuration? since that term configuration is overloaded. core/src/main/java/org/apache/sqoop/authentication/AuthenticationConstants.java <https://reviews.apache.org/r/26678/#comment98568> please document this enum ( javadoc ) and indent it. core/src/main/java/org/apache/sqoop/authentication/AuthenticationConstants.java <https://reviews.apache.org/r/26678/#comment98567> can we just have a static class, why do we need a constructor? core/src/main/java/org/apache/sqoop/authentication/AuthenticationError.java <https://reviews.apache.org/r/26678/#comment98570> why dont we move this unknown eror code to the base class, I see this c& p everywhere. can we have generic errors used across sqoop? Interested in knowign your thoughts https://issues.apache.org/jira/browse/SQOOP-1596 core/src/main/java/org/apache/sqoop/authentication/AuthenticationHandler.java <https://reviews.apache.org/r/26678/#comment98572> Please add java docs for api/ abstract classes core/src/main/java/org/apache/sqoop/authentication/AuthenticationHandler.java <https://reviews.apache.org/r/26678/#comment98574> remove thse superflous comments, it is pretty clear that it is logger no? core/src/main/java/org/apache/sqoop/authentication/AuthenticationHandler.java <https://reviews.apache.org/r/26678/#comment98571> shoud not security enabled be on by default? core/src/main/java/org/apache/sqoop/authentication/AuthenticationHandler.java <https://reviews.apache.org/r/26678/#comment98579> see comments below, I would rather have setting HADOOP_SECURITY_AUTHENTICATION as a method that sub classes can call core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment98564> please add java doc here. core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment98575> dont we need any thread safety around creation of these singletons? since I see a bunch of the methods been synchronized core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java <https://reviews.apache.org/r/26678/#comment98576> since there is nothing here, why are even overriding it? can we remove it. core/src/main/java/org/apache/sqoop/authentication/KerberosAuthenticationHandler.java <https://reviews.apache.org/r/26678/#comment98577> can we move all strings these to constants dist/src/main/server/conf/sqoop.properties <https://reviews.apache.org/r/26678/#comment98563> commented out? are these defaults? - Veena Basavaraj On Oct. 17, 2014, 1:53 a.m., richard zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26678/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 1:53 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/AuthenticationHandler.java > PRE-CREATION > > core/src/main/java/org/apache/sqoop/authentication/AuthenticationHandlerFactory.java > PRE-CREATION > > core/src/main/java/org/apache/sqoop/authentication/AuthenticationManager.java > PRE-CREATION > > core/src/main/java/org/apache/sqoop/authentication/KerberosAuthenticationHandler.java > PRE-CREATION > > core/src/main/java/org/apache/sqoop/authentication/SimpleAuthenticationHandler.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 > >
