sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Szabolcs Vasas <vasas.szabo...@gmail.com>
Subject Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should have an option to use binary mode for transfer
Date Fri, 13 Jul 2018 09:52:29 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review206041
-----------------------------------------------------------



Hi Chris,

Thank you for improving your patch, I think we made a big progress, your mainframe-related
code is now grouped to mainframe-related classes which reduced the risk of this patch.
I have left some comments, please fix these, I will have to continue my review with the core
classes and the test cases you added.

Szabolcs


src/java/org/apache/sqoop/SqoopOptions.java
Lines 2514 (patched)
<https://reviews.apache.org/r/62492/#comment289001>

    This method should have an Integer parameter and the casting should be done on the invoker
side.



src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Line 33 (original)
<https://reviews.apache.org/r/62492/#comment289002>

    You don't have any real changes in DataDrivenImportJob now so please restore it to the
original state. This will help tracking the patches adding real changes to a file later.



src/java/org/apache/sqoop/mapreduce/KeyRecordWriters.java
Lines 29 (patched)
<https://reviews.apache.org/r/62492/#comment288993>

    This class does not have to be generic.



src/java/org/apache/sqoop/mapreduce/KeyRecordWriters.java
Lines 35 (patched)
<https://reviews.apache.org/r/62492/#comment288995>

    This constant can be private.



src/java/org/apache/sqoop/mapreduce/KeyRecordWriters.java
Lines 67 (patched)
<https://reviews.apache.org/r/62492/#comment288994>

    As far as I understand this class should behave the same as the original RawKeyRecordWriter
except it is moved to to KeyRecordWriters and it extends GenericRecordWriter now.
    However in the original RawKeyRecordWriter the write and the close methods were synchronized
can we keep it that way? I am not sure why it was made like that but it is safer to keep it
that way.



src/java/org/apache/sqoop/mapreduce/RawKeyTextOutputFormat.java
Line 41 (original), 40 (patched)
<https://reviews.apache.org/r/62492/#comment288999>

    This method should be protected.



src/java/org/apache/sqoop/mapreduce/RawKeyTextOutputFormat.java
Line 80 (original), 48 (patched)
<https://reviews.apache.org/r/62492/#comment289000>

    This method should be protected.



src/java/org/apache/sqoop/tool/MainframeImportTool.java
Lines 43 (patched)
<https://reviews.apache.org/r/62492/#comment288997>

    This constant is never used.



src/java/org/apache/sqoop/tool/MainframeImportTool.java
Lines 86 (patched)
<https://reviews.apache.org/r/62492/#comment288996>

    This option is set in ImportTool as well, I think we should remove one of them, maybe
the one in ImportTool since it is only supported with mainframe?



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 45 (patched)
<https://reviews.apache.org/r/62492/#comment288998>

    Unused import.


- Szabolcs Vasas


On July 10, 2018, 12:42 a.m., Chris Teoh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
> 
> (Updated July 10, 2018, 12:42 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3224
>     https://issues.apache.org/jira/browse/SQOOP-3224
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> Added --as-binaryfile and --buffersize to support FTP transfer mode switching.
> 
> 
> Diffs
> -----
> 
>   build.xml 0ae729bc 
>   src/docs/user/import-mainframe.txt abeb7cde 
>   src/java/org/apache/sqoop/SqoopOptions.java 3a19aeac 
>   src/java/org/apache/sqoop/mapreduce/ByteKeyOutputFormat.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 349ca8d8 
>   src/java/org/apache/sqoop/mapreduce/KeyRecordWriters.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/RawKeyTextOutputFormat.java fec34f21 
>   src/java/org/apache/sqoop/mapreduce/mainframe/AbstractMainframeDatasetImportMapper.java
PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java ea54b07f

>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryImportMapper.java
PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryRecord.java PRE-CREATION

>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
1f78384b 
>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetImportMapper.java 0b7b5b85

>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java 8ef30d38 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 8d318327 
>   src/java/org/apache/sqoop/tool/ImportTool.java 25c3f703 
>   src/java/org/apache/sqoop/tool/MainframeImportTool.java 8883301d 
>   src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java 95bc0ecb 
>   src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java 041dfb78

>   src/test/org/apache/sqoop/manager/mainframe/MainframeTestUtil.java f28ff36c 
>   src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
PRE-CREATION 
>   src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
3547294f 
>   src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetPath.java 9b277b2a

>   src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeImportJob.java be62efd0

>   src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 0b0c6c34 
>   src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194 
> 
> 
> Diff: https://reviews.apache.org/r/62492/diff/19/
> 
> 
> Testing
> -------
> 
> Unit tests.
> 
> Functional testing on mainframe.
> 
> 
> Thanks,
> 
> Chris Teoh
> 
>


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