sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Szabolcs Vasas <vasas.szabo...@gmail.com>
Subject Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should have an option to use binary mode for transfer
Date Mon, 09 Jul 2018 10:45:19 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message