> On Aug. 25, 2014, 10 a.m., Jarek Cecho wrote: > > 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?
I cleaned up the spaces in the most recent version of the patch I posted in the Jira. Please refer to the latest patch for all documentation changes and provide feedback. > On Aug. 25, 2014, 10 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/manager/MainframeManager.java, line 50 > > <https://reviews.apache.org/r/22516/diff/1/?file=608148#file608148line50> > > > > Nit: Shoudn't this be private or protected constant? This is referred by a test to verify correctness. I am leaving it as public. > On Aug. 25, 2014, 10 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/manager/MainframeManager.java, lines 168-170 > > <https://reviews.apache.org/r/22516/diff/1/?file=608148#file608148line168> > > > > I'm assuming that this comment should be deleted? Correct. > On Aug. 25, 2014, 10 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/MainframeDatasetFTPRecordReader.java, > > lines 56-57 > > <https://reviews.apache.org/r/22516/diff/1/?file=608149#file608149line56> > > > > 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? I created a MainframeConfiguration class and defined the constant there. If it is deemed as an overkill, I can keep the new constant in DBConfiguration itself. Please provide feedback. > On Aug. 25, 2014, 10 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/MainframeDatasetImportMapper.java, > > lines 71-72 > > <https://reviews.apache.org/r/22516/diff/1/?file=608150#file608150line71> > > > > Please use constants from ConfigurationConstants class. Done in the recent patch. > On Aug. 25, 2014, 10 a.m., Jarek Cecho wrote: > > src/test/org/apache/sqoop/mapreduce/TestMainframeDatasetFTPRecordReader.java, > > lines 22-25 > > <https://reviews.apache.org/r/22516/diff/1/?file=608159#file608159line22> > > > > We should add mockito as dependency to ivy as well if we want to use > > this library. Added in the recent patch. - Mariappan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22516/#review51116 ----------------------------------------------------------- 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 > >
