sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boglarka Egyed <b...@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 12:45:55 GMT

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



Hi Chris,

I did a quick review and found a couple of things, please find them below.

Also I have pushed a change to trunk thus your patch doesn't apply there anymore. Could you
please rebase?

Thanks,
Bogi


src/java/org/apache/sqoop/SqoopOptions.java
Lines 2507 (patched)
<https://reviews.apache.org/r/62492/#comment286329>

    Could you please extract "" to a well named constant here?



src/java/org/apache/sqoop/mapreduce/KeyRecordWriters.java
Lines 1 (patched)
<https://reviews.apache.org/r/62492/#comment286330>

    Apache license header is missing from this file.



src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java
Lines 45 (patched)
<https://reviews.apache.org/r/62492/#comment286333>

    Trailing white spaces in this line will cause error during applying your patch.



src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
Lines 130 (patched)
<https://reviews.apache.org/r/62492/#comment286347>

    This method is still too long and complex, it is hard to understand what is its functionality.
Could you please split it up into smaller pieces so that each method would have only one responsibility
(having also signalized in its self-explanatory name)?



src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
Lines 131 (patched)
<https://reviews.apache.org/r/62492/#comment286336>

    To witch line does this comment belong to?



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
Lines 96-98 (patched)
<https://reviews.apache.org/r/62492/#comment286337>

    It might be just me but I would rethrow a RuntimeException here as if we hit catch block
it means something went wrong during the test, not the test case failed itself. Logging could
be also added, I think of something like this:
    
    catch (IOException ioe) {
          LOG.error("Issue with verifying binary record", ioe);
          throw new RuntimeException(ioe);
    }
    
    This comment applies to each relevant test cases.



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 53-54 (patched)
<https://reviews.apache.org/r/62492/#comment286344>

    Is there any spesific reason why these fields are not private?



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 196 (patched)
<https://reviews.apache.org/r/62492/#comment286338>

    Where do you use this variable?



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 203 (patched)
<https://reviews.apache.org/r/62492/#comment286339>

    Where do you use this variable?



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 225 (patched)
<https://reviews.apache.org/r/62492/#comment286345>

    This should be a constant, e.g. final int EXPECTED_BUFFER = 1024;



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 251-253 (patched)
<https://reviews.apache.org/r/62492/#comment286343>

    Why do you need these casting in these lines?



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 251-258 (patched)
<https://reviews.apache.org/r/62492/#comment286346>

    Untofrtunately I don't understand the intention here. What should this method do?


- Boglarka Egyed


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