----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22516/#review52127 -----------------------------------------------------------
I went through the code. It looks good. Thanks for taking the time to write this feature. I am not sure whether calling it MainframeImportTool is a proper name though. We currently support import from Z/OS DB2 also, so not sure whether this can confuse people. Also, this can be a specialization of a generic ftp record import method. That said, I am OK with the current names and formats if there are no further comments from others. src/java/org/apache/sqoop/SqoopOptions.java <https://reviews.apache.org/r/22516/#comment90872> Do we need to set tablename here - is it to overcome validations? src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java <https://reviews.apache.org/r/22516/#comment90873> Why can't we throw the IOExceptin and a finally clause to wrap the entire body of code to also disconnect from the FTP server. That will be clean and will cover all exit cases src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java <https://reviews.apache.org/r/22516/#comment90874> Same as above. src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java <https://reviews.apache.org/r/22516/#comment90875> Throwing the exception here with the ioe as the cause would provide better diagnostics in stack traces (For example, throw new IOException("....", ioe); - Venkat Ranganathan On Aug. 27, 2014, 7:55 p.m., Mariappan Asokan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22516/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2014, 7:55 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-trunk > > > Description > ------- > > This is to move mainframe datasets to Hadoop. > > > Diffs > ----- > > ivy.xml 6335e01 > ivy/libraries.properties 6818b3e > src/docs/man/common-args.txt e8d1f17 > src/docs/man/database-connection-args.txt PRE-CREATION > src/docs/man/import-args.txt 2bb69ba > src/docs/man/import-common-args.txt PRE-CREATION > src/docs/man/mainframe-connection-args.txt PRE-CREATION > src/docs/man/sqoop-import-mainframe.txt PRE-CREATION > src/docs/man/sqoop-import.txt 00b1ec8 > src/docs/man/sqoop.txt febe827 > src/docs/user/SqoopUserGuide.txt 2e88887 > src/docs/user/basics.txt 7e5a76a > src/docs/user/connecting-to-mainframe.txt PRE-CREATION > src/docs/user/distributed-cache.txt PRE-CREATION > src/docs/user/import-mainframe-purpose.txt PRE-CREATION > src/docs/user/import-mainframe.txt PRE-CREATION > src/docs/user/import.txt c5ffa50 > src/docs/user/intro.txt 99cd475 > src/docs/user/mainframe-common-args.txt PRE-CREATION > src/docs/user/tools.txt 7d977d4 > src/docs/user/validation-args.txt 3cb5f66 > src/java/org/apache/sqoop/SqoopOptions.java 3ef5a97 > src/java/org/apache/sqoop/manager/MainframeManager.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetImportMapper.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetInputFormat.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetInputSplit.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetRecordReader.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java > PRE-CREATION > src/java/org/apache/sqoop/tool/MainframeImportTool.java PRE-CREATION > src/java/org/apache/sqoop/tool/SqoopTool.java dbe429a > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java PRE-CREATION > src/test/org/apache/sqoop/manager/TestMainframeManager.java PRE-CREATION > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java > PRE-CREATION > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetInputFormat.java > PRE-CREATION > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetInputSplit.java > PRE-CREATION > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeImportJob.java > PRE-CREATION > src/test/org/apache/sqoop/tool/TestMainframeImportTool.java PRE-CREATION > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/22516/diff/ > > > Testing > ------- > > > Thanks, > > Mariappan Asokan > >
