----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62523/#review210537 -----------------------------------------------------------
Hi Chris, Thanks for the update, MainframeManagerImportTest passes now. I have however a couple of new findings regarding your latest update and in general regarding to extra FTP commands usage, please find them below. Thank you, Bogi src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java Lines 86-90 (patched) <https://reviews.apache.org/r/62523/#comment295259> This block is embedded under the condition of 'if (SqoopOptions.FileLayout.BinaryFile == options.getFileLayout())'. Does this mean that these extra commands can work only with binary file? If yes, could you please explain why? src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 56 (patched) <https://reviews.apache.org/r/62523/#comment295256> You never use DEFAULT_FTP_URL anywhere. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Line 120 (original), 129 (patched) <https://reviews.apache.org/r/62523/#comment295257> I think clean up of these lines has been missed. You are setting the username twice now. This applies for the other test cases too. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 192-194 (original), 197-199 (patched) <https://reviews.apache.org/r/62523/#comment295258> These should have been cleaned up too I guess. This applies for the other test cases too. - Boglarka Egyed On Nov. 13, 2018, 11:24 p.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62523/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2018, 11:24 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3237 > https://issues.apache.org/jira/browse/SQOOP-3237 > > > Repository: sqoop-trunk > > > Description > ------- > > Added --ftpcmds command to allow comma separated list of FTP commands to send. > > > Diffs > ----- > > src/docs/user/import-mainframe.txt 3ecfb7e4 > src/java/org/apache/sqoop/SqoopOptions.java f06872f9 > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java > 9842daa6 > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java > 90dc2ddd > src/java/org/apache/sqoop/tool/MainframeImportTool.java fbc8c3db > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java e7c48a6b > src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 00e57bd0 > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 0714bdcf > > > Diff: https://reviews.apache.org/r/62523/diff/9/ > > > Testing > ------- > > Unit tests. > > > File Attachments > ---------------- > > SQOOP-3237-1.patch > > https://reviews.apache.org/media/uploaded/files/2017/09/26/56041556-e355-4372-83ab-1bcc01680201__SQOOP-3237-1.patch > > > Thanks, > > Chris Teoh > >
