----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62492/#review206235 -----------------------------------------------------------
Hi Chris, I took a look at the current version of your patch and had several findings, please find them below. Unfortunately I couldn't get through the whole diff, I will iterate over it again later. Also, applying your patch throws warnings: /Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:52: trailing whitespace. In the case of generation data group datasets, Sqoop will retrieve just the last or /Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:61: trailing whitespace. Support for binary datasets by specifying --as-binaryfile and optionally --buffersize followed by /Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:63: trailing whitespace. will alter the number of records Sqoop reports to have imported. This is because it reads the /Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:73: trailing whitespace. Import of a tape based generation data group dataset using a password alias and writing out to /Users/boglarkaegyed/Downloads/SQOOP-3224-25.patch:81: trailing whitespace. Import of a tape based binary generation data group dataset with a buffer size of 64000 using a warning: squelched 1 whitespace error warning: 6 lines add whitespace errors. Could you please correct these? Thanks, Bogi src/docs/user/import-mainframe.txt Lines 197 (patched) <https://reviews.apache.org/r/62492/#comment289124> What does the "-" character mean at the end in this line? Also, could you please use the word "option" instead of "parameter" here? src/java/org/apache/sqoop/mapreduce/mainframe/AbstractMainframeDatasetImportMapper.java Lines 35 (patched) <https://reviews.apache.org/r/62492/#comment289136> There is no logging made in the code currently, LOG is unused. src/java/org/apache/sqoop/mapreduce/mainframe/AbstractMainframeDatasetImportMapper.java Lines 39 (patched) <https://reviews.apache.org/r/62492/#comment289137> Could you please use a more self-explanatory name here? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryRecord.java Lines 41 (patched) <https://reviews.apache.org/r/62492/#comment289138> There is no logging made in the code currently, LOG is unused. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java Lines 59-62 (patched) <https://reviews.apache.org/r/62492/#comment289125> Input parameters inputSplit and taskAttemptContext are unused here and InterruptedException seems to be unnecessary too. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java Lines 123 (patched) <https://reviews.apache.org/r/62492/#comment289126> ClassNotFoundException seems to be unnecessary here. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java Lines 124 (patched) <https://reviews.apache.org/r/62492/#comment289127> In line 77 and 108 you used equals method instead of ==, could you please stay consistent? src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 41 (patched) <https://reviews.apache.org/r/62492/#comment289139> In general I suggest to use full names instead of abbreviations for better understanding. src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 46-48 (patched) <https://reviews.apache.org/r/62492/#comment289140> These could be local variables. src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 49-50 (patched) <https://reviews.apache.org/r/62492/#comment289141> These could be private. src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 66-67 (patched) <https://reviews.apache.org/r/62492/#comment289142> Why don't you use it DATASET_NAME here instead of "dummy.ds"? src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 79-81 (patched) <https://reviews.apache.org/r/62492/#comment289145> Variables bytes and len are unused here. src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 98 (patched) <https://reviews.apache.org/r/62492/#comment289143> This could be simplified to assertEquals(). src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java Lines 136 (patched) <https://reviews.apache.org/r/62492/#comment289146> This could be simplified to assertEquals(). src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 194-252 (patched) <https://reviews.apache.org/r/62492/#comment289130> Exception handling is not consistent here. You have org.apache.sqoop.SqoopOptions and org.apache.sqoop.SqoopOptiopns.InvalidOptionException imported too and yet you use the following expressions after "throws": - org.apache.sqoop.SqoopOptiopns.InvalidOptionException - SqoopOptiopns.InvalidOptionException - InvalidOptionException Could you please use InvalidOptionException everywhere? src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 201 (patched) <https://reviews.apache.org/r/62492/#comment289131> This test case and testAsBinaryFileSetsCorrectFileLayoutAndDefaultBufferSize tests the same scenario. Could you please keep only one of them with all the assertions needed? src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 248 (patched) <https://reviews.apache.org/r/62492/#comment289132> You can use InvalidOptionException without "SqoopOption." here too. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 367 (patched) <https://reviews.apache.org/r/62492/#comment289135> Could you please explain what this test case is testing? I don't understand the assertions at the end. Desn't it test that sendCommand return what you have set on the mockFTPCLient? How does this test that binary mode has been set properly? - Boglarka Egyed On July 10, 2018, 12:42 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62492/ > ----------------------------------------------------------- > > (Updated July 10, 2018, 12:42 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 > ----- > > build.xml 0ae729bc > src/docs/user/import-mainframe.txt abeb7cde > src/java/org/apache/sqoop/SqoopOptions.java 3a19aeac > src/java/org/apache/sqoop/mapreduce/ByteKeyOutputFormat.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 349ca8d8 > 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 > 8ef30d38 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 8d318327 > src/java/org/apache/sqoop/tool/ImportTool.java 25c3f703 > src/java/org/apache/sqoop/tool/MainframeImportTool.java 8883301d > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java 95bc0ecb > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > 041dfb78 > src/test/org/apache/sqoop/manager/mainframe/MainframeTestUtil.java f28ff36c > > 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/mapreduce/mainframe/TestMainframeDatasetPath.java > 9b277b2a > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeImportJob.java > be62efd0 > 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/19/ > > > Testing > ------- > > Unit tests. > > Functional testing on mainframe. > > > Thanks, > > Chris Teoh > >
