steveloughran commented on a change in pull request #1952:
URL: https://github.com/apache/hadoop/pull/1952#discussion_r411272114
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FTPFileSystem.java
##########
@@ -340,8 +343,19 @@ public FSDataOutputStream create(Path file, FsPermission
permission,
// file. The FTP client connection is closed when close() is called on the
// FSDataOutputStream.
client.changeWorkingDirectory(parent.toUri().getPath());
- FSDataOutputStream fos = new FSDataOutputStream(client.storeFileStream(file
- .getName()), statistics) {
+ OutputStream outputStream = client.storeFileStream(file.getName());
+
+ if (!FTPReply.isPositivePreliminary(client.getReplyCode())) {
+ // The ftpClient is an inconsistent state. Must close the stream
+ // which in turn will logout and disconnect from FTP server
+ if (outputStream != null) {
+ outputStream.close();
Review comment:
could this raise an IOE? If so, that disconnect() afterwards still needs
to be called, so make close() a catch/log operation. IOUtils.closeStream could
do this (and it includes the null check)
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FTPFileSystem.java
##########
@@ -110,7 +111,9 @@ public void initialize(URI uri, Configuration conf) throws
IOException { // get
// get port information from uri, (overrides info in conf)
int port = uri.getPort();
- port = (port == -1) ? FTP.DEFAULT_PORT : port;
+ if(port == -1){
+ port = conf.getInt(FS_FTP_HOST_PORT, FTP.DEFAULT_PORT);
Review comment:
assuming we have documentation for the FTP connector, you are going to
have to document this new option.
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/ftp/TestFTPFileSystem.java
##########
@@ -37,9 +54,70 @@
*/
public class TestFTPFileSystem {
+ private FtpTestServer server;
+
@Rule
public Timeout testTimeout = new Timeout(180000);
+ @Before
+ public void setUp() throws Exception {
+ server = new FtpTestServer(GenericTestUtils.getTestDir().toPath()).start();
+ }
+
+ @After
+ @SuppressWarnings("ResultOfMethodCallIgnored")
+ public void tearDown() throws Exception {
+ server.stop();
Review comment:
handle case where server==null, i.e. setup failed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
