sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 22516: Support importing mainframe sequential datasets
Date Mon, 25 Aug 2014 10:00:18 GMT

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


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