----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62492/#review192493 -----------------------------------------------------------
src/java/org/apache/sqoop/mapreduce/BinaryKeyOutputFormat.java Lines 1 (patched) <https://reviews.apache.org/r/62492/#comment270639> Can you please add license information to this new file? src/java/org/apache/sqoop/mapreduce/BinaryKeyOutputFormat.java Lines 2 (patched) <https://reviews.apache.org/r/62492/#comment270646> It seems that big part of this class is a duplication of the content of RawKeyTextOutputFormat. Can we eliminate the duplication? src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java Line 86 (original), 89 (patched) <https://reviews.apache.org/r/62492/#comment270645> Do you think we could introduce a new FileLayout value instead of transfer mode? Is the file layout used in mainframe import commands? If yes and we keep the mainframeFtpTransferMode option as well it could be confusing if both of them are specified. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryImportMapper.java Lines 1 (patched) <https://reviews.apache.org/r/62492/#comment270640> Can you please add license information to this new file? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryImportMapper.java Lines 2 (patched) <https://reviews.apache.org/r/62492/#comment270647> Some parts of this class is a duplicate of code MainframeDatasetImportMapper, can you please fix that? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryRecord.java Lines 1 (patched) <https://reviews.apache.org/r/62492/#comment270642> Can you please add license information to this new file? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java Lines 75 (patched) <https://reviews.apache.org/r/62492/#comment270648> This method is almost the exact duplicate of the previous initialize method, can you please fix it? src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 1 (patched) <https://reviews.apache.org/r/62492/#comment270643> Can you please add license information to this new file? - Szabolcs Vasas On Nov. 29, 2017, 11:48 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62492/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2017, 11:48 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3224 > https://issues.apache.org/jira/browse/SQOOP-3224 > > > Repository: sqoop-trunk > > > Description > ------- > > Added --transfermode {ascii|binary} to support FTP transfer mode switching. > > > Diffs > ----- > > src/java/org/apache/sqoop/SqoopOptions.java 587d4e1d > src/java/org/apache/sqoop/mapreduce/BinaryKeyOutputFormat.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java dc49282e > 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/MainframeImportJob.java > f222dc8f > src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java 95bc0ecb > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java > PRE-CREATION > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java > 06141549 > src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194 > > > Diff: https://reviews.apache.org/r/62492/diff/7/ > > > Testing > ------- > > Unit tests. > > Functional testing on mainframe. > > > Thanks, > > Chris Teoh > >
