sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mariappan Asokan" <maso...@syncsort.com>
Subject Re: Review Request 22516: Support importing mainframe sequential datasets
Date Wed, 27 Aug 2014 19:48:52 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message