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 Wed, 22 Nov 2017 16:41:24 GMT

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




src/java/org/apache/sqoop/mapreduce/BinaryKeyOutputFormat.java
Lines 15 (patched)
<https://reviews.apache.org/r/62492/#comment269611>

    Please avoid wild card import usage and use exact imports here.



src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
Lines 96 (patched)
<https://reviews.apache.org/r/62492/#comment269619>

    Please do not use org.apache.hadoop.conf.Configuration.DBConfiguration as it is deprecated
and will be removed soon, please use org.apache.sqoop.mapreduce.db.DBConfiguration instead
here.



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

    This method is indeed quite complex, could you make it more modular by extracting specific
parts into smaller methods with self-explanatory names?



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

    Please remove the unnecessary space here between byte and [].



src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
Lines 153-156 (patched)
<https://reviews.apache.org/r/62492/#comment269613>

    This code part is used more than once, could you please extract it into a method?



src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
Lines 169-170 (patched)
<https://reviews.apache.org/r/62492/#comment269616>

    Please use throw new IOException("IOException during data transfer: ", ioe) instead as
it will print the whole stack trace as well.



src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
Lines 187-191 (patched)
<https://reviews.apache.org/r/62492/#comment269617>

    Would you mind to create two constructs (one with ftp = MainframeFTPClientUtils.getFTPConnection(conf)
and one where ftp would be setable) instead of creating two public setters just for testing
purposes?



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Line 210 (original), 211 (patched)
<https://reviews.apache.org/r/62492/#comment269621>

    This comment is not completely correct after your change.



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 296-401 (patched)
<https://reviews.apache.org/r/62492/#comment269624>

    Could you please create a test class for MainframeDatasetBinaryRecord class and put all
these test case into that instead of here? Also these test cases duplicate many code lines,
could you please extract common part to method(s) to make it more readable?



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 311 (patched)
<https://reviews.apache.org/r/62492/#comment269627>

    Please use lower camel case at every new test case as well.



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 326 (patched)
<https://reviews.apache.org/r/62492/#comment269626>

    Please ioe instead of ioe.toString() at every new test cases as well.



src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
Lines 190-244 (patched)
<https://reviews.apache.org/r/62492/#comment269632>

    These test cases duplicate a lot of code. Could you please extract common cases to method(s)
to make the tests more readable?



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

    Please use org.apache.sqoop.SqoopOptions.InvalidOptionsException instead of deprecated
com.cloudera.sqoop.SqoopOptions.InvalidOptionsException in every new test cases.



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

    Please use the non-deprecated versions of ToolOptions and SqoopOptions in every test cases
too.



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

    This line is no more needed as you are using the expectedException logic.



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 368-369 (patched)
<https://reviews.apache.org/r/62492/#comment269631>

    I would rather make these constants.


- Boglarka Egyed


On Nov. 17, 2017, 6:36 a.m., Chris Teoh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2017, 6:36 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/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/6/
> 
> 
> Testing
> -------
> 
> Unit tests.
> 
> Functional testing on mainframe.
> 
> 
> Thanks,
> 
> Chris Teoh
> 
>


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