You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Chris Teoh <ch...@gmail.com> on 2018/06/18 01:47:31 UTC
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/
-----------------------------------------------------------
(Updated June 18, 2018, 11:47 a.m.)
Review request for Sqoop.
Changes
-------
Added integration tests.
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 (updated)
-----
build.xml 0ae729bc
src/docs/user/import-mainframe.txt abeb7cde
src/java/org/apache/sqoop/SqoopOptions.java d9984af3
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 3b542102
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 c62ee98c
src/java/org/apache/sqoop/tool/ImportTool.java 2c474b7e
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/tool/TestMainframeImportTool.java 0b0c6c34
src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194
Diff: https://reviews.apache.org/r/62492/diff/17/
Changes: https://reviews.apache.org/r/62492/diff/16-17/
Testing
-------
Unit tests.
Functional testing on mainframe.
Thanks,
Chris Teoh
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review205364
-----------------------------------------------------------
Hi Chris,
Since this is a quite big patch I will review it iteratively.
In the first iteration I would like you to move the mainframe-related code to the mainframe classes since your changes affect the mainframe connector only, please see my comments below.
I have also seen many unused imports added by your changes, please remove those. I have started to add separate comment for every unused import but the list is not complete please check the other classes too.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 40 (patched)
<https://reviews.apache.org/r/62492/#comment288261>
Unused import.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 43 (patched)
<https://reviews.apache.org/r/62492/#comment288262>
Unnecessary new line.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 55 (patched)
<https://reviews.apache.org/r/62492/#comment288263>
Unused import.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 57 (patched)
<https://reviews.apache.org/r/62492/#comment288264>
Unused import.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 58 (patched)
<https://reviews.apache.org/r/62492/#comment288265>
Unused import.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 59 (patched)
<https://reviews.apache.org/r/62492/#comment288266>
Unused import.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Line 91 (original), 100 (patched)
<https://reviews.apache.org/r/62492/#comment288268>
Since the BinaryFile layout is only supported by the mainframe connector we should move this branch to org.apache.sqoop.mapreduce.mainframe.MainframeImportJob#configureMapper.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 205 (patched)
<https://reviews.apache.org/r/62492/#comment288269>
Since the BinaryFile layout is only supported by the mainframe connector we should move this branch to org.apache.sqoop.mapreduce.mainframe.MainframeImportJob#getOutputFormatClass.
src/java/org/apache/sqoop/mapreduce/RawKeyTextOutputFormat.java
Lines 70 (patched)
<https://reviews.apache.org/r/62492/#comment288270>
This behavior should be moved to another class since it is really unexpected in a class called RawKeyTextOutputFormat.
I think you should introduce a new class for mainframe output format and if the format is text then you just delegate to an instance of RawKeyTextOutputFormat otherwise you return an instance of BinaryKeyRecordWriter.
- Szabolcs Vasas
On June 18, 2018, 1:47 a.m., Chris Teoh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 1:47 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 d9984af3
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 3b542102
> 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 c62ee98c
> src/java/org/apache/sqoop/tool/ImportTool.java 2c474b7e
> 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/tool/TestMainframeImportTool.java 0b0c6c34
> src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194
>
>
> Diff: https://reviews.apache.org/r/62492/diff/17/
>
>
> Testing
> -------
>
> Unit tests.
>
> Functional testing on mainframe.
>
>
> Thanks,
>
> Chris Teoh
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Szabolcs Vasas <va...@gmail.com>.
> On Aug. 17, 2018, 12:56 p.m., Szabolcs Vasas wrote:
> > Hi Chris,
> >
> > Thank you for the improvements, I have not found any major issues with your patch, so I think if you fix the below small things we will be good to go.
> >
> > Thanks,
> > Szabolcs
And I have ran the test and third party tests successfully with your patch.
- Szabolcs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review207485
-----------------------------------------------------------
On Aug. 13, 2018, 10:52 a.m., Chris Teoh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2018, 10:52 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 084823cf
> src/docs/user/import-mainframe.txt abeb7cde
> src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
> 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 9dcbdd59
> src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
> src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
> 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/20/
>
>
> Testing
> -------
>
> Unit tests.
>
> Functional testing on mainframe.
>
>
> Thanks,
>
> Chris Teoh
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Chris Teoh <ch...@gmail.com>.
> On Aug. 17, 2018, 10:56 p.m., Szabolcs Vasas wrote:
> > Hi Chris,
> >
> > Thank you for the improvements, I have not found any major issues with your patch, so I think if you fix the below small things we will be good to go.
> >
> > Thanks,
> > Szabolcs
>
> Szabolcs Vasas wrote:
> And I have ran the test and third party tests successfully with your patch.
Thanks for your time to review Szabolcs. I have updated the patch based on your review. Please let me know if there are further changes.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review207485
-----------------------------------------------------------
On Aug. 13, 2018, 8:52 p.m., Chris Teoh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2018, 8:52 p.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 084823cf
> src/docs/user/import-mainframe.txt abeb7cde
> src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
> 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 9dcbdd59
> src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
> src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
> 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/20/
>
>
> Testing
> -------
>
> Unit tests.
>
> Functional testing on mainframe.
>
>
> Thanks,
>
> Chris Teoh
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review207485
-----------------------------------------------------------
Hi Chris,
Thank you for the improvements, I have not found any major issues with your patch, so I think if you fix the below small things we will be good to go.
Thanks,
Szabolcs
src/java/org/apache/sqoop/SqoopOptions.java
Lines 34 (patched)
<https://reviews.apache.org/r/62492/#comment290886>
Unused import, please remove it.
src/java/org/apache/sqoop/tool/MainframeImportTool.java
Line 74 (original)
<https://reviews.apache.org/r/62492/#comment290885>
This is an unrelated whitespace change, please restore the original state.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 46 (patched)
<https://reviews.apache.org/r/62492/#comment290888>
I don't see any other changes in this class so please restore its original state.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetPath.java
Line 24 (original)
<https://reviews.apache.org/r/62492/#comment290889>
I don't see any other change in this class so please restore the original state.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeImportJob.java
Line 27 (original)
<https://reviews.apache.org/r/62492/#comment290890>
I don't see any other change in this class so please restore the original state.
src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 22 (patched)
<https://reviews.apache.org/r/62492/#comment290887>
Unused import.
- Szabolcs Vasas
On Aug. 13, 2018, 10:52 a.m., Chris Teoh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2018, 10:52 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 084823cf
> src/docs/user/import-mainframe.txt abeb7cde
> src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
> 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 9dcbdd59
> src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
> src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
> 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/20/
>
>
> Testing
> -------
>
> Unit tests.
>
> Functional testing on mainframe.
>
>
> Thanks,
>
> Chris Teoh
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review207667
-----------------------------------------------------------
Fix it, then Ship it!
Hi Chris,
Thank you for improving your patch!
I have some very minor findings regarding the latest version otherwise it LGTM in general, the tests passed too.
Thanks,
Bogi
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Line 40 (original)
<https://reviews.apache.org/r/62492/#comment291154>
Seems to be an unnecessary line deletion, this file doesn't contain anz other relevant change. Please revert.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Line 51 (original)
<https://reviews.apache.org/r/62492/#comment291155>
Seems to be an unnecessary line deletion, this file doesn't contain anz other relevant change. Please revert.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 46-47 (patched)
<https://reviews.apache.org/r/62492/#comment291251>
Unnecessary new lines, there is no other change in this file, please revert.
- Boglarka Egyed
On Aug. 17, 2018, 1:39 p.m., Chris Teoh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 1:39 p.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 084823cf
> src/docs/user/import-mainframe.txt abeb7cde
> src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
> 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 9dcbdd59
> src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
> src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
> 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/tool/TestMainframeImportTool.java 0b0c6c34
>
>
> Diff: https://reviews.apache.org/r/62492/diff/21/
>
>
> Testing
> -------
>
> Unit tests.
>
> Functional testing on mainframe.
>
>
> Thanks,
>
> Chris Teoh
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review207814
-----------------------------------------------------------
Ship it!
Ship It!
- Szabolcs Vasas
On Aug. 23, 2018, 9:51 a.m., Chris Teoh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 9:51 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 084823cf
> src/docs/user/import-mainframe.txt abeb7cde
> src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
> src/java/org/apache/sqoop/mapreduce/ByteKeyOutputFormat.java PRE-CREATION
> 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 9dcbdd59
> src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
> src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
> 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/tool/TestMainframeImportTool.java 0b0c6c34
>
>
> Diff: https://reviews.apache.org/r/62492/diff/22/
>
>
> Testing
> -------
>
> Unit tests.
>
> Functional testing on mainframe.
>
>
> Thanks,
>
> Chris Teoh
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Chris Teoh <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/
-----------------------------------------------------------
(Updated Aug. 23, 2018, 7:51 p.m.)
Review request for Sqoop.
Changes
-------
Based on feedback.
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 (updated)
-----
build.xml 084823cf
src/docs/user/import-mainframe.txt abeb7cde
src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
src/java/org/apache/sqoop/mapreduce/ByteKeyOutputFormat.java PRE-CREATION
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 9dcbdd59
src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
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/tool/TestMainframeImportTool.java 0b0c6c34
Diff: https://reviews.apache.org/r/62492/diff/22/
Changes: https://reviews.apache.org/r/62492/diff/21-22/
Testing
-------
Unit tests.
Functional testing on mainframe.
Thanks,
Chris Teoh
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Chris Teoh <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 11:39 p.m.)
Review request for Sqoop.
Changes
-------
Updated based on feedback.
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 (updated)
-----
build.xml 084823cf
src/docs/user/import-mainframe.txt abeb7cde
src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
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 9dcbdd59
src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
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/tool/TestMainframeImportTool.java 0b0c6c34
Diff: https://reviews.apache.org/r/62492/diff/21/
Changes: https://reviews.apache.org/r/62492/diff/20-21/
Testing
-------
Unit tests.
Functional testing on mainframe.
Thanks,
Chris Teoh
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Chris Teoh <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/
-----------------------------------------------------------
(Updated Aug. 13, 2018, 8:52 p.m.)
Review request for Sqoop.
Changes
-------
Updated based on feedback.
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 (updated)
-----
build.xml 084823cf
src/docs/user/import-mainframe.txt abeb7cde
src/java/org/apache/sqoop/SqoopOptions.java f97dbfdf
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 9dcbdd59
src/java/org/apache/sqoop/tool/ImportTool.java 478f1748
src/java/org/apache/sqoop/tool/MainframeImportTool.java cdd9d6d0
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/20/
Changes: https://reviews.apache.org/r/62492/diff/19-20/
Testing
-------
Unit tests.
Functional testing on mainframe.
Thanks,
Chris Teoh
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review206235
-----------------------------------------------------------
Hi Chris,
I took a look at the current version of your patch and had several findings, please find them below. Unfortunately I couldn't get through the whole diff, I will iterate over it again later.
Also, applying your patch throws warnings:
/Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:52: trailing whitespace.
In the case of generation data group datasets, Sqoop will retrieve just the last or
/Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:61: trailing whitespace.
Support for binary datasets by specifying --as-binaryfile and optionally --buffersize followed by
/Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:63: trailing whitespace.
will alter the number of records Sqoop reports to have imported. This is because it reads the
/Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:73: trailing whitespace.
Import of a tape based generation data group dataset using a password alias and writing out to
/Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:81: trailing whitespace.
Import of a tape based binary generation data group dataset with a buffer size of 64000 using a
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.
Could you please correct these?
Thanks,
Bogi
src/docs/user/import-mainframe.txt
Lines 197 (patched)
<https://reviews.apache.org/r/62492/#comment289124>
What does the "-" character mean at the end in this line? Also, could you please use the word "option" instead of "parameter" here?
src/java/org/apache/sqoop/mapreduce/mainframe/AbstractMainframeDatasetImportMapper.java
Lines 35 (patched)
<https://reviews.apache.org/r/62492/#comment289136>
There is no logging made in the code currently, LOG is unused.
src/java/org/apache/sqoop/mapreduce/mainframe/AbstractMainframeDatasetImportMapper.java
Lines 39 (patched)
<https://reviews.apache.org/r/62492/#comment289137>
Could you please use a more self-explanatory name here?
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryRecord.java
Lines 41 (patched)
<https://reviews.apache.org/r/62492/#comment289138>
There is no logging made in the code currently, LOG is unused.
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
Lines 59-62 (patched)
<https://reviews.apache.org/r/62492/#comment289125>
Input parameters inputSplit and taskAttemptContext are unused here and InterruptedException seems to be unnecessary too.
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java
Lines 123 (patched)
<https://reviews.apache.org/r/62492/#comment289126>
ClassNotFoundException seems to be unnecessary here.
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java
Lines 124 (patched)
<https://reviews.apache.org/r/62492/#comment289127>
In line 77 and 108 you used equals method instead of ==, could you please stay consistent?
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 41 (patched)
<https://reviews.apache.org/r/62492/#comment289139>
In general I suggest to use full names instead of abbreviations for better understanding.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 46-48 (patched)
<https://reviews.apache.org/r/62492/#comment289140>
These could be local variables.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 49-50 (patched)
<https://reviews.apache.org/r/62492/#comment289141>
These could be private.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 66-67 (patched)
<https://reviews.apache.org/r/62492/#comment289142>
Why don't you use it DATASET_NAME here instead of "dummy.ds"?
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 79-81 (patched)
<https://reviews.apache.org/r/62492/#comment289145>
Variables bytes and len are unused here.
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 98 (patched)
<https://reviews.apache.org/r/62492/#comment289143>
This could be simplified to assertEquals().
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 136 (patched)
<https://reviews.apache.org/r/62492/#comment289146>
This could be simplified to assertEquals().
src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 194-252 (patched)
<https://reviews.apache.org/r/62492/#comment289130>
Exception handling is not consistent here. You have org.apache.sqoop.SqoopOptions and org.apache.sqoop.SqoopOptiopns.InvalidOptionException imported too and yet you use the following expressions after "throws":
- org.apache.sqoop.SqoopOptiopns.InvalidOptionException
- SqoopOptiopns.InvalidOptionException
- InvalidOptionException
Could you please use InvalidOptionException everywhere?
src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 201 (patched)
<https://reviews.apache.org/r/62492/#comment289131>
This test case and testAsBinaryFileSetsCorrectFileLayoutAndDefaultBufferSize tests the same scenario. Could you please keep only one of them with all the assertions needed?
src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 248 (patched)
<https://reviews.apache.org/r/62492/#comment289132>
You can use InvalidOptionException without "SqoopOption." here too.
src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 367 (patched)
<https://reviews.apache.org/r/62492/#comment289135>
Could you please explain what this test case is testing? I don't understand the assertions at the end. Desn't it test that sendCommand return what you have set on the mockFTPCLient? How does this test that binary mode has been set properly?
- Boglarka Egyed
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
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Chris Teoh <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/
-----------------------------------------------------------
(Updated July 10, 2018, 10:42 a.m.)
Review request for Sqoop.
Changes
-------
Thanks for your review Szabolcs. I have updated the patch based on feedback.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/62492/diff/18-19/
Testing
-------
Unit tests.
Functional testing on mainframe.
Thanks,
Chris Teoh
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review205846
-----------------------------------------------------------
Hi Chris,
I have done a quick review on your patch, please find my comments below.
I can still see unused imports in your patch, please make sure you remove those before submitting the next version.
Szabolcs
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Line 56 (original), 51 (patched)
<https://reviews.apache.org/r/62492/#comment288746>
Unused import.
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java
Lines 108 (patched)
<https://reviews.apache.org/r/62492/#comment288747>
I think you introduce unnecessary code duplication here, since in case of Text file this method works the same as its super method.
I think the code duplication could be eliminated like this:
@Override
protected void configureMapper(Job job, String tableName,
String tableClassName) throws IOException {
super.configureMapper(job, tableName, tableClassName);
if (SqoopOptions.FileLayout.BinaryFile.equals(options.getFileLayout())) {
job.setOutputKeyClass(BytesWritable.class);
job.setOutputValueClass(NullWritable.class);
// this is required as code generated class assumes setField method takes String
// and will fail with ClassCastException when a byte array is passed instead
// java.lang.ClassCastException: [B cannot be cast to java.lang.String
Configuration conf = job.getConfiguration();
conf.setClass(org.apache.sqoop.mapreduce.db.DBConfiguration.INPUT_CLASS_PROPERTY, MainframeDatasetBinaryRecord.class,
DBWritable.class);
}
}
src/java/org/apache/sqoop/mapreduce/ByteKeyOutputFormat.java
Lines 39 (patched)
<https://reviews.apache.org/r/62492/#comment288748>
This class duplicates lots of code from RawKeyTextOutputFormat.
Do you support different compression codecs when importing in binary formats? If no, then you could eliminate the code handling the compression, otherwise please refactor this to eliminate the code duplication.
- Szabolcs Vasas
On July 3, 2018, 6:21 a.m., Chris Teoh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
>
> (Updated July 3, 2018, 6:21 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/tool/TestMainframeImportTool.java 0b0c6c34
> src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194
>
>
> Diff: https://reviews.apache.org/r/62492/diff/18/
>
>
> Testing
> -------
>
> Unit tests.
>
> Functional testing on mainframe.
>
>
> Thanks,
>
> Chris Teoh
>
>
Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should
have an option to use binary mode for transfer
Posted by Chris Teoh <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/
-----------------------------------------------------------
(Updated July 3, 2018, 4:21 p.m.)
Review request for Sqoop.
Changes
-------
As per feedback.
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 (updated)
-----
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/tool/TestMainframeImportTool.java 0b0c6c34
src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194
Diff: https://reviews.apache.org/r/62492/diff/18/
Changes: https://reviews.apache.org/r/62492/diff/17-18/
Testing
-------
Unit tests.
Functional testing on mainframe.
Thanks,
Chris Teoh