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