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