> On Dec. 1, 2017, 3:25 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java > > Line 86 (original), 89 (patched) > > <https://reviews.apache.org/r/62492/diff/7/?file=1903517#file1903517line89> > > > > 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.
The file layout is not used in the import commands. For this particular scenario it's only FTP so it's either ascii or binary transfer modes. The default setting for the transfer is TextFile. I can add another FileLayout though would it be automatically set to BinaryFile or RawFile when the transfer mode is set? > On Dec. 1, 2017, 3:25 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryImportMapper.java > > Lines 2 (patched) > > <https://reviews.apache.org/r/62492/diff/7/?file=1903519#file1903519line2> > > > > Some parts of this class is a duplicate of code > > MainframeDatasetImportMapper, can you please fix that? Thanks for the review. This class uses "extends AutoProgressMapper<LongWritable, SqoopRecord, BytesWritable, NullWritable>" instead of "extends AutoProgressMapper<LongWritable, SqoopRecord, Text, NullWritable>" in MainframeDatasetImportMapper class. Do you have suggestions on how to reduce the duplication? > On Dec. 1, 2017, 3:25 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java > > Lines 75 (patched) > > <https://reviews.apache.org/r/62492/diff/7/?file=1903521#file1903521line76> > > > > This method is almost the exact duplicate of the previous initialize > > method, can you please fix it? This is initialize method is used for testing only, the problem with the original method is there's a call to getConfiguration() in the parent class I cannot mock. Do you have any suggestions? - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62492/#review192493 ----------------------------------------------------------- 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 > >
