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