----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22516/#review51116 -----------------------------------------------------------
Hi Asokan, thank you very much for taking up this huge change! I do have couple of high level comments: 1) It seems that you have several "trailing whitespaces" in the patch. Could you please clean them up? They will show up with a red mark here on review board, so they are easy to spot. 2) It seems that there are no docs - could you please add small paragraph to our user guide documenting the behaviour? src/java/org/apache/sqoop/manager/MainframeManager.java <https://reviews.apache.org/r/22516/#comment89502> Nit: Shoudn't this be private or protected constant? src/java/org/apache/sqoop/manager/MainframeManager.java <https://reviews.apache.org/r/22516/#comment89141> I'm assuming that this comment should be deleted? src/java/org/apache/sqoop/mapreduce/MainframeDatasetFTPRecordReader.java <https://reviews.apache.org/r/22516/#comment89503> I don't particulary like the fact that we're reusing existing constant for table name for something else. What about creating a new constant for the mainframe dataset name? src/java/org/apache/sqoop/mapreduce/MainframeDatasetImportMapper.java <https://reviews.apache.org/r/22516/#comment89678> Please use constants from ConfigurationConstants class. src/test/org/apache/sqoop/mapreduce/TestMainframeDatasetFTPRecordReader.java <https://reviews.apache.org/r/22516/#comment89501> We should add mockito as dependency to ivy as well if we want to use this library. Jarcec - Jarek Cecho On June 14, 2014, 10:46 p.m., Mariappan Asokan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22516/ > ----------------------------------------------------------- > > (Updated June 14, 2014, 10:46 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-trunk > > > Description > ------- > > This is to move mainframe datasets to Hadoop. > > > Diffs > ----- > > src/java/org/apache/sqoop/manager/MainframeManager.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/MainframeDatasetFTPRecordReader.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/MainframeDatasetImportMapper.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/MainframeDatasetInputFormat.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/MainframeDatasetInputSplit.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/MainframeDatasetRecordReader.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/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/TestMainframeDatasetFTPRecordReader.java > PRE-CREATION > src/test/org/apache/sqoop/mapreduce/TestMainframeDatasetInputFormat.java > PRE-CREATION > src/test/org/apache/sqoop/mapreduce/TestMainframeDatasetInputSplit.java > PRE-CREATION > src/test/org/apache/sqoop/mapreduce/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 > >
