sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Szabolcs Vasas <vasas.szabo...@gmail.com>
Subject Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer
Date Mon, 03 Dec 2018 09:23:08 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message