Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Dec. 13, 2018, 9:56 a.m.)


Review request for Sqoop.


Changes
---

Removed whitespace and unused imports.


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 (updated)
-

  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 fc6e56d6 


Diff: https://reviews.apache.org/r/62523/diff/12/

Changes: https://reviews.apache.org/r/62523/diff/11-12/


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Fero Szabo via Review Board]

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


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)


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)


Also missing space around the + operators.

LOG.info("ReplyCode: " + res + " ReplyString: " + result);



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 59 (patched)


This new line is unnecessary here.



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 21-22 (patched)


Unused import



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 34 (patched)


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/#review211139
---


Ship it!




Ship It!

- Szabolcs Vasas


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

> On Dec. 3, 2018, 8:23 p.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
> > Lines 343 (patched)
> > 
> >
> > 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");
> >   }
> > ```

Many thanks for the suggestion. Yes it is to ensure we can send those FTP 
commands. I have adjusted as recommended.


> On Dec. 3, 2018, 8:23 p.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
> > Lines 359-360 (patched)
> > 
> >
> > MainframeFTPClientUtils.getFTPConnection already invokes 
> > applyFtpCommands, so you don't have to invoke it again.

Thanks again. It is included in the suggested unit test fix from the previous 
issue.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/#review210979
---


On Dec. 10, 2018, 4:45 p.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62523/
> ---
> 
> (Updated Dec. 10, 2018, 4:45 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/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
> 
>

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Dec. 10, 2018, 4:45 p.m.)


Review request for Sqoop.


Changes
---

Updated docs and unit tests as per review feedback.


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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/62523/diff/10-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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Szabolcs Vasas]

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


It should be --as-binaryfile instead of ---as-binaryfile



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 57 (patched)


Unnecessary white space change.



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 60-61 (patched)


Unused imports.



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 343 (patched)


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)


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)


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)


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Nov. 29, 2018, 10:46 p.m.)


Review request for Sqoop.


Changes
---

Changes based on feedback. Removed requirement to use --as-binaryfiles to use 
--ftp-commands


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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/62523/diff/9-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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

> On Nov. 8, 2018, 9:34 p.m., Boglarka Egyed wrote:
> > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java
> > Lines 86 (patched)
> > 
> >
> > Shouldn't this condition be negated? StringUtils.isBalnk will be true 
> > if the character sequence is null, empty or whitespace. I'm wondering how 
> > TestMainframeImportTool could pass with your patch.

Apologies you are right. I have updated this.


> On Nov. 8, 2018, 9:34 p.m., Boglarka Egyed wrote:
> > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
> > Lines 383-393 (patched)
> > 
> >
> > These configurations could be extracted to a separate method having a 
> > self-explanatory name.

Yes I have refactored it out to a method and revised the unit tests.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/#review210413
---


On Nov. 14, 2018, 10:24 a.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62523/
> ---
> 
> (Updated Nov. 14, 2018, 10:24 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/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
> 
>

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Boglarka Egyed]

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


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)


You never use DEFAULT_FTP_URL anywhere.



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Line 120 (original), 129 (patched)


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)


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Nov. 14, 2018, 10:24 a.m.)


Review request for Sqoop.


Changes
---

Corrected logic, and fixed unit tests after refactor.


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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/62523/diff/8-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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Boglarka Egyed]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/#review210413
---



Hi Chris,

Many thanks for your continued efforts on Mainframe side!

I took a look at your patch and found a bug, please find it below. 
MainframeManagerImportTest failed for me with the following error message, I 
believe the root cause is the bug you introduced with the latest version of 
your patch:

ERROR - org.apache.sqoop.tool.ImportTool.run(ImportTool.java:635)] Import 
failed: The value of property mainframe.ftp.commands must not be null

Could you please take a look?

Thanks,
Bogi


src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java
Lines 86 (patched)


Shouldn't this condition be negated? StringUtils.isBalnk will be true if 
the character sequence is null, empty or whitespace. I'm wondering how 
TestMainframeImportTool could pass with your patch.



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 383-393 (patched)


These configurations could be extracted to a separate method having a 
self-explanatory name.


- Boglarka Egyed


On Oct. 31, 2018, 5:40 a.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62523/
> ---
> 
> (Updated Oct. 31, 2018, 5:40 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/tool/TestMainframeImportTool.java 00e57bd0 
>   src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 0714bdcf 
> 
> 
> Diff: https://reviews.apache.org/r/62523/diff/8/
> 
> 
> 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
> 
>

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

> On Oct. 19, 2018, 12:06 a.m., Szabolcs Vasas wrote:
> > 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

Thanks for the review Szabolcs. I have also included documentation in this diff.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/#review209738
---


On Oct. 31, 2018, 4:40 p.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62523/
> ---
> 
> (Updated Oct. 31, 2018, 4:40 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/7/
> 
> 
> 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
> 
>

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

> On Oct. 19, 2018, 12:06 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
> > Lines 223 (patched)
> > 
> >
> > 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.

At the moment, we only log the reply strings. Yes, I can add it into the 
documentation.


> On Oct. 19, 2018, 12:06 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/tool/TestMainframeImportTool.java
> > Lines 241 (patched)
> > 
> >
> > You should use the assert methods in org.junit.Assert instead of the 
> > assert statement.
> > Please fix all occurrences.

Do you also want me to fix occurrences in this class not related to this change?


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/#review209738
---


On Oct. 31, 2018, 4:40 p.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62523/
> ---
> 
> (Updated Oct. 31, 2018, 4:40 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/7/
> 
> 
> 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
> 
>

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Oct. 31, 2018, 4:40 p.m.)


Review request for Sqoop.


Changes
---

Renamed --ftpcmds to --ftp-commands along with review feedback adjustments.


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 (updated)
-

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

Changes: https://reviews.apache.org/r/62523/diff/6-7/


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Szabolcs Vasas]

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


You can simplify this expression by using 
org.apache.commons.lang3.StringUtils#isBlank



src/java/org/apache/sqoop/tool/MainframeImportTool.java
Lines 45 (patched)


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)


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)


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)


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)


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Aug. 31, 2018, 11:56 p.m.)


Review request for Sqoop.


Changes
---

Rebased on trunk after 3224 commit


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.

This patch supercedes SQOOP-3224 as it includes it.


Diffs (updated)
-

  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/

Changes: https://reviews.apache.org/r/62523/diff/5-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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Sept. 28, 2017, 12:50 a.m.)


Review request for Sqoop.


Changes
---

Updated description. SQOOP-3224 patch is superceded by this one as it includes 
SQOOP-3224 changes.


Bugs: SQOOP-3237
https://issues.apache.org/jira/browse/SQOOP-3237


Repository: sqoop-trunk


Description (updated)
---

Added --ftpcmds command to allow comma separated list of FTP commands to send.

This patch supercedes SQOOP-3224 as it includes it.


Diffs
-

  src/java/org/apache/sqoop/SqoopOptions.java 587d4e1d 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
ea54b07f 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df 


Diff: https://reviews.apache.org/r/62523/diff/5/


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Sept. 28, 2017, 12:47 a.m.)


Review request for Sqoop.


Changes
---

Breaking larger patch into smaller patches. This one has a dependency on 
SQOOP-3224 to be committed first


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 (updated)
-

  src/java/org/apache/sqoop/SqoopOptions.java 587d4e1d 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
ea54b07f 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df 


Diff: https://reviews.apache.org/r/62523/diff/5/

Changes: https://reviews.apache.org/r/62523/diff/4-5/


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Sept. 26, 2017, 11:19 p.m.)


Review request for Sqoop.


Changes
---

Added dependent SQOOP JIRAs


Bugs: SQOOP-3224, SQOOP-3225 and SQOOP-3237
https://issues.apache.org/jira/browse/SQOOP-3224
https://issues.apache.org/jira/browse/SQOOP-3225
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 587d4e1d 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
ea54b07f 
  
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileEntryParser.java 
f0b87868 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 
  
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileEntryParser.java
 eb0f8c00 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df 


Diff: https://reviews.apache.org/r/62523/diff/4/


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Sept. 26, 2017, 6:50 a.m.)


Review request for Sqoop.


Changes
---

Added bug number to JIRA


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 587d4e1d 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
ea54b07f 
  
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileEntryParser.java 
f0b87868 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 
  
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileEntryParser.java
 eb0f8c00 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df 


Diff: https://reviews.apache.org/r/62523/diff/4/


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Sept. 26, 2017, 6:23 a.m.)


Review request for Sqoop.


Changes
---

Added trim command to remove extraneous whitespace for FTP commands


Repository: sqoop-trunk


Description
---

Added --ftpcmds command to allow comma separated list of FTP commands to send.


Diffs (updated)
-

  src/java/org/apache/sqoop/SqoopOptions.java 587d4e1d 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
ea54b07f 
  
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileEntryParser.java 
f0b87868 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 
  
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileEntryParser.java
 eb0f8c00 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df 


Diff: https://reviews.apache.org/r/62523/diff/4/

Changes: https://reviews.apache.org/r/62523/diff/3-4/


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Sept. 26, 2017, 6:11 a.m.)


Review request for Sqoop.


Changes
---

Rolled up patch to include SQOOP-3224 and SQOOP-3225.


Repository: sqoop-trunk


Description
---

Added --ftpcmds command to allow comma separated list of FTP commands to send.


Diffs (updated)
-

  src/java/org/apache/sqoop/SqoopOptions.java 587d4e1d 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
ea54b07f 
  
src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileEntryParser.java 
f0b87868 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 
  
src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileEntryParser.java
 eb0f8c00 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df 


Diff: https://reviews.apache.org/r/62523/diff/3/

Changes: https://reviews.apache.org/r/62523/diff/2-3/


Testing
---

Unit tests.


File Attachments (updated)


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

Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Chris Teoh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/
---

(Updated Sept. 23, 2017, 11:51 a.m.)


Review request for Sqoop.


Changes
---

Updated JIRA number


Summary (updated)
-

SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior 
to transfer


Repository: sqoop-trunk


Description
---

Added --ftpcmds command to allow comma separated list of FTP commands to send.


Diffs (updated)
-

  src/java/org/apache/sqoop/SqoopOptions.java 2eb3d8a4 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
ea54b07f 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java 0cb91dbc 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java d51e33e5 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df 


Diff: https://reviews.apache.org/r/62523/diff/2/

Changes: https://reviews.apache.org/r/62523/diff/1-2/


Testing
---

Unit tests.


Thanks,

Chris Teoh