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