----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62523/#review209738 -----------------------------------------------------------
Hi Chris, Thank you for submitting this new feature implementations! On the high level it seems good to me, I had some comments on the code style, please see them below. Apart from those I think you should document this new option to let other people know that it exists. Regards, Szabolcs src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java Lines 85 (patched) <https://reviews.apache.org/r/62523/#comment294290> You can simplify this expression by using org.apache.commons.lang3.StringUtils#isBlank src/java/org/apache/sqoop/tool/MainframeImportTool.java Lines 45 (patched) <https://reviews.apache.org/r/62523/#comment294291> I think we could use a more descriptive name here e.g. ftp-commands src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Lines 223 (patched) <https://reviews.apache.org/r/62523/#comment294292> Just a question here: do we want to do something with the reply strings? I case one of the commands cause an error we do not throw exception here which could be fine but I think we need to document it. src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Lines 269-304 (patched) <https://reviews.apache.org/r/62523/#comment294293> These methods are a bit complex and can be greatly simplified by using some Java 8 features. Please see my suggestion in this commit: https://github.com/szvasas/sqoop/commit/652de7b8f26a595423d5f7095dca12128d782cfc?diff=split It is a good practice to return an empty colletion/array instead of null because then later you can avoid NullPointerExceptions and use for-each loops much easier. src/test/org/apache/sqoop/tool/TestMainframeImportTool.java Lines 241 (patched) <https://reviews.apache.org/r/62523/#comment294294> You should use the assert methods in org.junit.Assert instead of the assert statement. Please fix all occurrences. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 423 (patched) <https://reviews.apache.org/r/62523/#comment294295> It would be better to assert on the whole content of the array not just the size. See my suggestion in this commit: https://github.com/szvasas/sqoop/commit/652de7b8f26a595423d5f7095dca12128d782cfc?diff=split#diff-ffd210901f618bfeecf90bd4d4c3c2dfR424 - Szabolcs Vasas On Aug. 31, 2018, 1:56 p.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62523/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2018, 1:56 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/java/org/apache/sqoop/SqoopOptions.java f06872f9 > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java > 9d6a2fe7 > 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 654721e3 > src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 00e57bd0 > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194 > > > Diff: https://reviews.apache.org/r/62523/diff/6/ > > > 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 > >
