----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25090/#review54357 -----------------------------------------------------------
Hey Venkat. Just a few comments and concerns! src/java/org/apache/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/25090/#comment94486> Maybe add some spacing before "new"? src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java <https://reviews.apache.org/r/25090/#comment94492> No need for a condition here. You can just have: return !(clCredProvider == null || clsCredProviderFactory == null || methCreateCredEntry == null || methGetPassword == null || methFlush == null); src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java <https://reviews.apache.org/r/25090/#comment94495> RuntimeException? src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java <https://reviews.apache.org/r/25090/#comment94494> RuntimeException? src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java <https://reviews.apache.org/r/25090/#comment94493> This doesn't seem like an IOException. Why not just a RuntimeException or a custom exception that extends RuntimException? src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java <https://reviews.apache.org/r/25090/#comment94496> Same thoughts as above src/java/org/apache/sqoop/util/password/CredentialProviderPasswordLoader.java <https://reviews.apache.org/r/25090/#comment94497> Instead of creating inheritance hierarchies, why not pull out verifyPath and readBytes into a utility class. Then CredentialProviderPasswordLoader can extend PasswordLoader. src/java/org/apache/sqoop/util/password/CredentialProviderPasswordLoader.java <https://reviews.apache.org/r/25090/#comment94498> The exception message here will not make sense if the file doesn't exist: "The password file does not exist". - Abraham Elmahrek On Sept. 23, 2014, 7:18 a.m., Venkat Ranganathan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25090/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2014, 7:18 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1471 > https://issues.apache.org/jira/browse/SQOOP-1471 > > > Repository: sqoop-trunk > > > Description > ------- > > Use the Hadoop Credential Provider facility to use password aliases instead > of password so that password need not be embedded in clear text in scipts and > password files. > > Enhanced both the password on the command line with a password-alias opton > and also the ability to store an alias instead of clear text in the password > file option. > > Used reflection to call the APIs instead of directly accessing them so that > we don't have hard dependency on Hadoop 2.6.x > > Added documentation updates > > Made sure no new checkstyle violations are there > > > Diffs > ----- > > src/docs/user/connecting.txt 6a28254 > src/java/org/apache/sqoop/SqoopOptions.java d16ccb3 > src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 0ac35de > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 26950cc > src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java > PRE-CREATION > > src/java/org/apache/sqoop/util/password/CredentialProviderPasswordLoader.java > PRE-CREATION > src/test/org/apache/sqoop/credentials/TestPassingSecurePassword.java > bbf82f4 > > Diff: https://reviews.apache.org/r/25090/diff/ > > > Testing > ------- > > Tested with Hadoop versions 2.4 and 1.x to make sure we fail gracefully if > the facility is not available. Tested with 2.6.0 snapshot build that was > built by myself and also against the SNAPSHOT builds posted to validate the > functionality. > > Added two new tests to test the functionality > > > Thanks, > > Venkat Ranganathan > >
