----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62523/#review210979 -----------------------------------------------------------
Hi Chris, Thank you for the improvements! I think the patch will be OK, I have found some minor issues, please see them below. src/docs/user/import-mainframe.txt Lines 228 (patched) <https://reviews.apache.org/r/62523/#comment295811> It should be --as-binaryfile instead of ---as-binaryfile src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java Lines 57 (patched) <https://reviews.apache.org/r/62523/#comment295849> Unnecessary white space change. src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java Lines 60-61 (patched) <https://reviews.apache.org/r/62523/#comment295813> Unused imports. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 343 (patched) <https://reviews.apache.org/r/62523/#comment295850> I think the purpose of this test is to verify that if you create the FTP connection, the init commands are executed, right? In that case it would be better to modify it to use Mockito's verify functionality: ``` @Test public void testFtpCommandExecutes() throws IOException { final String EXPECTED_RESPONSE = "200 OK"; final int EXPECTED_RESPONSE_CODE = 200; String ftpcmds = "quote SITE RDW,quote SITE RDW READTAPEFORMAT=V"; final int FTP_CMD_COUNT = 2; when(mockFTPClient.login("user", "pssword")).thenReturn(true); when(mockFTPClient.logout()).thenReturn(true); when(mockFTPClient.isConnected()).thenReturn(false); when(mockFTPClient.getReplyCode()).thenReturn(EXPECTED_RESPONSE_CODE); when(mockFTPClient.getReplyString()).thenReturn(EXPECTED_RESPONSE); setupDefaultConfiguration(); conf.set(MainframeConfiguration.MAINFRAME_INPUT_DATASET_TYPE,"g"); conf.set(MainframeConfiguration.MAINFRAME_INPUT_DATASET_NAME,"a.b.c.d"); conf.set(MainframeConfiguration.MAINFRAME_FTP_TRANSFER_MODE,MainframeConfiguration.MAINFRAME_FTP_TRANSFER_MODE_BINARY); conf.set(MainframeConfiguration.MAINFRAME_FTP_CUSTOM_COMMANDS, ftpcmds); MainframeFTPClientUtils.setMockFTPClient(mockFTPClient); MainframeFTPClientUtils.getFTPConnection(conf); verify(mockFTPClient).sendCommand("quote SITE RDW"); verify(mockFTPClient).sendCommand("quote SITE RDW READTAPEFORMAT=V"); } ``` src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 359-360 (patched) <https://reviews.apache.org/r/62523/#comment295851> MainframeFTPClientUtils.getFTPConnection already invokes applyFtpCommands, so you don't have to invoke it again. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 401 (patched) <https://reviews.apache.org/r/62523/#comment295852> It would be more straightforward to check if the result array is empty here: assertEquals(0, cmds.length) src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 408 (patched) <https://reviews.apache.org/r/62523/#comment295853> It would be more straightforward to check if the result array is empty here: assertEquals(0, cmds.length) - Szabolcs Vasas On Nov. 29, 2018, 11:46 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62523/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2018, 11:46 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/10/ > > > 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 > >
