You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by christeoh <gi...@git.apache.org> on 2018/03/19 10:19:49 UTC
[GitHub] sqoop pull request #44: [SQOOP-3224] Binary transfer mode
GitHub user christeoh opened a pull request:
https://github.com/apache/sqoop/pull/44
[SQOOP-3224] Binary transfer mode
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/christeoh/sqoop 3224-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/sqoop/pull/44.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #44
----
----
---
[GitHub] sqoop pull request #44: [SQOOP-3224] Binary transfer mode
Posted by fszabo2 <gi...@git.apache.org>.
Github user fszabo2 commented on a diff in the pull request:
https://github.com/apache/sqoop/pull/44#discussion_r175483531
--- Diff: src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java ---
@@ -207,8 +208,18 @@ public static FTPClient getFTPConnection(Configuration conf)
throw new IOException("Could not login to server " + server
+ ":" + ftp.getReplyString());
}
- // set ASCII transfer mode
- ftp.setFileType(FTP.ASCII_FILE_TYPE);
+ // set transfer mode
+ String transferMode = conf.get(MainframeConfiguration.MAINFRAME_FTP_TRANSFER_MODE);
--- End diff --
The whole getFTPConnection method is already very long and does a bunch of steps. You could consider refactoring it into smaller methods for readability.
---
[GitHub] sqoop pull request #44: [SQOOP-3224] Binary transfer mode
Posted by christeoh <gi...@git.apache.org>.
Github user christeoh closed the pull request at:
https://github.com/apache/sqoop/pull/44
---
[GitHub] sqoop pull request #44: [SQOOP-3224] Binary transfer mode
Posted by christeoh <gi...@git.apache.org>.
Github user christeoh commented on a diff in the pull request:
https://github.com/apache/sqoop/pull/44#discussion_r176717474
--- Diff: src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java ---
@@ -207,8 +208,18 @@ public static FTPClient getFTPConnection(Configuration conf)
throw new IOException("Could not login to server " + server
+ ":" + ftp.getReplyString());
}
- // set ASCII transfer mode
- ftp.setFileType(FTP.ASCII_FILE_TYPE);
+ // set transfer mode
+ String transferMode = conf.get(MainframeConfiguration.MAINFRAME_FTP_TRANSFER_MODE);
--- End diff --
I'm not sure how you would refactor it. I can give it a try and see if it is what you're looking for.
---
[GitHub] sqoop pull request #44: [SQOOP-3224] Binary transfer mode
Posted by fszabo2 <gi...@git.apache.org>.
Github user fszabo2 commented on a diff in the pull request:
https://github.com/apache/sqoop/pull/44#discussion_r175484508
--- Diff: src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java ---
@@ -362,4 +363,29 @@ public void testPartitionedDatasetsShouldReturnAllFiles() {
Assert.fail(ioeString);
}
}
+ @Test
+ public void testBinaryTransferMode() throws IOException {
+ final String EXPECTED_RESPONSE = "200 Representation type is Image";
+ final int EXPECTED_RESPONSE_CODE = 200;
+ when(mockFTPClient.login("user", "pssword")).thenReturn(true);
--- End diff --
is the typo "pssword" intentional? Though won't cause any trouble I guess...
---
[GitHub] sqoop pull request #44: [SQOOP-3224] Binary transfer mode
Posted by fszabo2 <gi...@git.apache.org>.
Github user fszabo2 commented on a diff in the pull request:
https://github.com/apache/sqoop/pull/44#discussion_r175478607
--- Diff: src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java ---
@@ -33,4 +33,13 @@
public static final String MAINFRAME_INPUT_DATASET_TAPE = "mainframe.input.dataset.tape";
public static final String MAINFRAME_FTP_FILE_ENTRY_PARSER_CLASSNAME = "org.apache.sqoop.mapreduce.mainframe.MainframeFTPFileEntryParser";
+
+ public static final String MAINFRAME_FTP_TRANSFER_MODE = "mainframe.ftp.transfermode";
+
+ public static final String MAINFRAME_FTP_TRANSFER_MODE_ASCII = "ascii";
+
+ public static final String MAINFRAME_FTP_TRANSFER_MODE_BINARY = "binary";
+
+ // this is the buffer size used when doing binary ftp transfers from mainframe
+ public static final Integer MAINFRAME_FTP_TRANSFER_BINARY_BUFFER = 32760;
--- End diff --
Is this value always a good choice? I would make it configurable (if it isn't already), and use this value as default if said configuration is not present, so the user has a choice here.
---