> On Sept. 23, 2014, 11:24 p.m., Abraham Elmahrek wrote: > > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, lines 1039-1041 > > <https://reviews.apache.org/r/25090/diff/2/?file=702639#file702639line1039> > > > > Maybe add some spacing before "new"?
Fixed. > On Sept. 23, 2014, 11:24 p.m., Abraham Elmahrek wrote: > > src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java, > > lines 91-98 > > <https://reviews.apache.org/r/25090/diff/2/?file=702640#file702640line91> > > > > No need for a condition here. You can just have: > > > > return !(clCredProvider == null || clsCredProviderFactory == null || > > methCreateCredEntry == null || methGetPassword == null || methFlush == > > null); I understand from aesthetics point of view (particularly how we would rather do in dynamic languages). But we follow this in rest of the codebase, for consistency I intend to leave it as this. let me know if you feel strongly. > On Sept. 23, 2014, 11:24 p.m., Abraham Elmahrek wrote: > > src/java/org/apache/sqoop/util/password/CredentialProviderPasswordLoader.java, > > line 33 > > <https://reviews.apache.org/r/25090/diff/2/?file=702641#file702641line33> > > > > Instead of creating inheritance hierarchies, why not pull out > > verifyPath and readBytes into a utility class. Then > > CredentialProviderPasswordLoader can extend PasswordLoader. CredentialProviderPasswordLoader is a File loader, where the file contains the encrypted password. So, we would depend on the file password loader functionality also. Sorry if I missed something > On Sept. 23, 2014, 11:24 p.m., Abraham Elmahrek wrote: > > src/java/org/apache/sqoop/util/password/CredentialProviderPasswordLoader.java, > > line 54 > > <https://reviews.apache.org/r/25090/diff/2/?file=702641#file702641line54> > > > > The exception message here will not make sense if the file doesn't > > exist: "The password file does not exist". I see your point. I don't think I modified the verifypath code (that comes from FilePasswordLoader. Similar issue with the cannot be a directory. Let me update those also to meaningful. Something like. Sounds good? The provided password file <file> does not exist The provided password file <file> is a directory - Venkat ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25090/#review54357 ----------------------------------------------------------- On Oct. 7, 2014, 11:27 p.m., Venkat Ranganathan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25090/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2014, 11:27 p.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/tool/BaseSqoopTool.java 498ad79 > src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java > PRE-CREATION > > src/java/org/apache/sqoop/util/password/CredentialProviderPasswordLoader.java > PRE-CREATION > src/java/org/apache/sqoop/util/password/FilePasswordLoader.java 4a288bf > 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 > >
