[GitHub] [hadoop] steveloughran commented on a change in pull request #1952: HDFS-1820. FTPFileSystem attempts to close the outputstream even when it is not initialised.

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org