sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fero Szabo via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should have an option to use binary mode for transfer
Date Tue, 29 May 2018 08:56:25 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review203967
-----------------------------------------------------------



Hi Chris,

I had a quick glance at your patch. Mainly, it seems to be fine for me, with some minor issues.


I'm trying to get new Sqoop features documented. Since this is a new feature, can you please
add documentation to the project? Just a couple of sentences and one or two examples should
suffice!
The file to modify is located under src/docs. (It's probably src/docs/user/import-mainframe.txt.)

It's unclear to me how I could better test it. I wonder how difficult would it be for me to
emulate a mainframe on a virtual machine and somehow integrate that with sqoop. Certainly
sounds like a lot of effort! If more of these patches are coming, than we might benefit from
an integration test environment like this.


src/java/org/apache/sqoop/tool/MainframeImportTool.java
Lines 90 (patched)
<https://reviews.apache.org/r/62492/#comment286334>

    What is the unit of the buffersize? 
    Your example suggests that it's kB. 
    Can you please specify it in the description?



src/java/org/apache/sqoop/tool/MainframeImportTool.java
Lines 218 (patched)
<https://reviews.apache.org/r/62492/#comment286324>

    Looks like incorrect parenthesis to me.
    
    Shouldn't it be?
    if (SqoopOptions.FileLayout.BinaryFile.equals(options.getFileLayout()) &&
      (
      options.getMainframeInputDatasetName() == null 
      || options.getMainframeInputDatasetName().equals("")
      )
    ) {
    ...
    
    Anyway, I recommend using org.apache.commons.lang3.StringUtils.isEmpty() to check if a
String is null or empty



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 213 (patched)
<https://reviews.apache.org/r/62492/#comment286328>

    Why the extra parenthesis around (transfermode)? :)



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 215 (patched)
<https://reviews.apache.org/r/62492/#comment286331>

    Just asking out of curiosity, do you know the root cause for this?



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 221 (patched)
<https://reviews.apache.org/r/62492/#comment286332>

    Would it make sense to rather throw an exception here and force the user to specify the
encoding? 
    
    Though I'm not too familiar with FTP... Does it always default to ASCII? In that case,
I admit that it would make sense to use this as the default.


- Fero Szabo


On May 29, 2018, 8:05 a.m., Chris Teoh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
> 
> (Updated May 29, 2018, 8:05 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
> -----
> 
>   src/java/org/apache/sqoop/SqoopOptions.java d9984af3 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java a5962ba4 
>   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 7e975c7b 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 783651a4 
>   src/java/org/apache/sqoop/tool/ImportTool.java ee79d8b7 
>   src/java/org/apache/sqoop/tool/MainframeImportTool.java 8883301d 
>   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
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/11/
> 
> 
> Testing
> -------
> 
> Unit tests.
> 
> Functional testing on mainframe.
> 
> 
> Thanks,
> 
> Chris Teoh
> 
>


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