----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62492/#review190724 -----------------------------------------------------------
Hi Chris, Thank you for your contribution! I have started to review your patch but since it is a big change I think I will do it in chunks and I will need to understand some of the design ideas better. Please see my initial findings and questions below. src/java/org/apache/sqoop/SqoopOptions.java Lines 2421 (patched) <https://reviews.apache.org/r/62492/#comment268314> Please set the default value in the initDefaults() method. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryRecord.java Lines 24 (patched) <https://reviews.apache.org/r/62492/#comment268316> Can you please explain what is the purpose of this new class? The name of its field is not too descriptive, is it supposed to contain the content of one file on the mainframe? I am not too familiar with how the Mainframe connector works, why does the binary import need a SqoopRecord implementation and text import doesn't? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java Lines 24 (patched) <https://reviews.apache.org/r/62492/#comment268317> Please remove the star import and import the necessary classes only. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java Lines 125 (patched) <https://reviews.apache.org/r/62492/#comment268318> This method is quite complex and it is not covered with test cases. Can you please fill this gap? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java Lines 128 (patched) <https://reviews.apache.org/r/62492/#comment268319> What is the purpose of this array, is it supposed to contain 1 record? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java Lines 138 (patched) <https://reviews.apache.org/r/62492/#comment268321> I think we should use some kind of buffered input stream. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java Lines 145 (patched) <https://reviews.apache.org/r/62492/#comment268322> Why do we need to specify the cumulativeBytesRead as the offset in the buffer array? src/java/org/apache/sqoop/tool/MainframeImportTool.java Lines 204 (patched) <https://reviews.apache.org/r/62492/#comment268323> We could use org.apache.commons.lang3.StringUtils#equalsIgnoreCase helper method which handles the null case as well. src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 199 (patched) <https://reviews.apache.org/r/62492/#comment268324> Please use a method of org.junit.Assert here src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 211 (patched) <https://reviews.apache.org/r/62492/#comment268325> Please use a method of org.junit.Assert here src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 224 (patched) <https://reviews.apache.org/r/62492/#comment268326> Please use a method of org.junit.Assert here src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 237 (patched) <https://reviews.apache.org/r/62492/#comment268329> Please use a method of org.junit.Assert here. src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 251 (patched) <https://reviews.apache.org/r/62492/#comment268328> Please use org.junit.rules.ExpectedException here. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 387 (patched) <https://reviews.apache.org/r/62492/#comment268330> Please use a method of org.junit.Assert here. - Szabolcs Vasas On Oct. 25, 2017, 10:25 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62492/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2017, 10:25 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/tool/TestMainframeImportTool.java d51e33e5 > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194 > > > Diff: https://reviews.apache.org/r/62492/diff/4/ > > > Testing > ------- > > Unit tests. > > Functional testing on mainframe. > > > Thanks, > > Chris Teoh > >
