----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62523/#review211186 -----------------------------------------------------------
Fix it, then Ship it! Hi Chris, I also had a look at this one. Nice, clean patch, good job! I just had a few comments on formatting, no biggies. We should probably automate this formatting stuff in the future somehow. Also the removal of unused imports. Best Regards, Fero src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java Lines 78 (patched) <https://reviews.apache.org/r/62523/#comment296113> You could also use isNotBlank next time :) That's slightly easier on my eyes. src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Lines 281 (patched) <https://reviews.apache.org/r/62523/#comment296115> Missing space. Should read like this: LOG.info("Issuing command: " + ftpCommand); I'm not sure if we have a formatting guidline like this, but we probably should if we don't. src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Lines 285 (patched) <https://reviews.apache.org/r/62523/#comment296116> Also missing space around the + operators. LOG.info("ReplyCode: " + res + " ReplyString: " + result); src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java Lines 59 (patched) <https://reviews.apache.org/r/62523/#comment296114> This new line is unnecessary here. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 21-22 (patched) <https://reviews.apache.org/r/62523/#comment296117> Unused import src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 34 (patched) <https://reviews.apache.org/r/62523/#comment296118> Unused import. - Fero Szabo On Dec. 10, 2018, 5:45 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62523/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2018, 5:45 a.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/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java > 502e6333 > src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 00e57bd0 > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java fc6e56d6 > > > Diff: https://reviews.apache.org/r/62523/diff/11/ > > > 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 > >
