Is it a dupe of HDFS-7005?

there is no write timeout. why not add writeTimeout?

I updated the title to make it clear that write timeout is still missing. HDFS-7005 already added read timeout.

Post a patch for DFSClient newConnectedPeer write timeout.

      socketTimeout = conf.getInt(DFS_CLIENT_SOCKET_TIMEOUT_KEY,
          HdfsServerConstants.READ_TIMEOUT);

Can we change this to use DFSConfigKeys#DFS_CLIENT_SOCKET_TIMEOUT_DEFAULT like the other configuration keys do, rather than HdfsServerConstants#READ_TIMEOUT?

Looks good other than that.

Thanks Colin P. McCabe for the review! I've updated patch based on the feedback.

Please let me know if I'm missing something, but it appears this patch would significantly alter the pre-existing write timeout behavior of the HDFS client.

Right now, write timeout is enforced not as a socket option, but instead enforced per operation by passing the timeout to SocketOutputStream, which uses it in the underlying NIO selector calls. The exact write timeout value is not purely based on configuration. It's also a function of the number of nodes in the write pipeline. The details are implemented in DFSClient#getDatanodeWriteTimeout. Under default configuration, this method would extend the configured timeout of 60 seconds to 75 seconds (additional 5 seconds per replica in the pipeline). Extending the timeout proportional to the pipeline size is meant to make the client robust against the cumulative latency effects of every write in the pipeline.

This patch would set a 60 second write timeout (under default configuration) directly as a socket option. I believe that effectively negates the extension time of up to 75 seconds that DFSClient#getDatanodeWriteTimeout was trying to allow.

I see the original problem reported in HDFS-7005 was related to lack of read timeout. I'm wondering if there is actually no further change required for write timeout, given the above explanation. Is anyone seeing an actual problem related to lack of write timeout?

Chris, I think you're absolutely right. I vaguely remembered that there was an alternate method of setting write timeouts we used in places, but I was unable to find it in a few minutes of digging. The fact that it's passed as a parameter to NetUtils#getOutputStream explains why looking for setWriteTimeout and similar didn't turn up anything.

However. I still think this is broken, because we will do some writes to the socket prior to calling DFSClient#getDataNodeWriteTimeout. For example, in RemoteBlockReader2#newBlockReader, we are writing stuff to the socket, all before ever calling DFSClient#getDataNodeWriteTimeout.

On a semi-related note, I think that the current configuration situation is highly confusing and unsatisfactory. We have a configuration key called simply dfs.client.socket-timeout, which doesn't specify whether it applies to reads or writes. I'm not even sure most HDFS developers could answer which one(s) this key does, if quizzed. Meanwhile, the units are unspecified (is it seconds? ms?) and the default value doesn't appear in DFSConfigKeys.java, unlike almost every other configuration key.

How about having dfs.client.datanode.socket.read.timeout.ms as an alias for dfs.client.socket-timeout,
dfs.client.datanode.socket.write.timeout.ms for a base write timeout, and dfs.client.datanode.socket.write.timeout.extra.per.pipeline.node.ms to be an extra amount that we add for each DN in the pipeline?

Colin P. McCabe, thanks for double-checking me on this. I'm also now starting to wonder if HDFS-7005 had unintended side effects. By setting read timeout as a socket option in DFSClient#newConnectedPeer, the setting also would have applied for DFSOutputStream, and thus circumvented the extension time that DFSClient#getDatanodeReadTimeout wants to apply.

For example, in RemoteBlockReader2#newBlockReader, we are writing stuff to the socket, all before ever calling DFSClient#getDataNodeWriteTimeout.

Yes, you're right. I suppose a complete implementation, with retention of the "timeout extension" behavior, is going to require pushing the NetUtils socket wrapping calls up to those earlier layers, or setting the socket option at a point where we know the number of nodes in the pipeline, and therefore can calculate the extension. I expect either of those are going to be much more invasive changes than the posted patch.

I'm not even sure most HDFS developers could answer which one(s) this key does, if quizzed.

I sure couldn't without reading the code fresh again today.

+1 for your proposal for new, clearer configuration properties.

-1 overall

This message was automatically generated.

+1 to revamp the timeout naming on DFSland, in the mean time I think this can be committed to trunk first and have another jira to deprecate the timeout properties.

attached new patch rebased to trunk. cc:Xiaoyu Yao

-1 overall

This message was automatically generated.

Esteban Gutierrez, thank you for rebasing, but this patch cannot be committed. As I described here, it would potentially change the existing behavior much more than intended.

https://issues.apache.org/jira/browse/HDFS-7608?focusedCommentId=14314868&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14314868

I'm clicking Cancel Patch to make it clearer that this patch isn't ready.

I agree with Chris on this one. We need to address the comments Colin P. McCabe and Chris Nauroth above before commit it. I also remove the Bugbash label to avoid confusion.

Thanks Chris Nauroth. I think just changing newConnectedPeer() to use the one provided by DFSClient.getDatanodeWriteTimeout() is good enough. In the DataStreamer we already use it and but we have set the number of nodes to 2. That should be fine and that gives at least the flexibility to tune it down if required. Also, I see that if we don't set a write timeout we can run into the issue that was mentioned in this JIRA and after adding the timeout in the peer I no longer experience this issue. I've noticed other issues like in Client() where we set up the connection and then the timeout but that can be addressed in another JIRA.

Chris Nauroth I tried the approach using DFSClient.getDatanodeWriteTimeout() and doesn't seem there is a clear way for users to use the timeout, e.g. if you pass a timeout of 1000ms then your final timeout will be 11000ms and that won't be straightforward. I think if you pass the socketTimeout is much more straightforward. More suggestions to address this are welcome Chris Nauroth.

Esteban Gutierrez, Chris Nauroth: I took another look at the patch and I think that it's correct as-is.

The confusing part about all of this is that the DFSInputStream uses the Peer classes to encapsulate sockets, whereas the DFSOutputStream simply uses "raw" socket objects. The word "Peer" does not occur in DFSOutputStream.java or DataStreamer.java. It only is used in the input stream code. Therefore, since this patch only changes newConnectedPeer, it only affects DFSInputStream.

In contrast, the issue Chris brought up is actually only a DFSOutputStream issue. Chris commented:

Right now, write timeout is enforced not as a socket option, but instead enforced per operation by passing the timeout to SocketOutputStream, which uses it in the underlying NIO selector calls. The exact write timeout value is not purely based on configuration. It's also a function of the number of nodes in the write pipeline. The details are implemented in DFSClient#getDatanodeWriteTimeout

But DFSClient#getDatanodeWriteTimeout is less generic than it may seem. It actually is only used by the DFSOutputStream (write path) code to calculate how long to set the timeout based on the number of nodes in the pipeline. It is never called by the input stream code at all. So I believe that this concern is not valid.

Chris asked:

I see the original problem reported in HDFS-7005 was related to lack of read timeout. I'm wondering if there is actually no further change required for write timeout, given the above explanation. Is anyone seeing an actual problem related to lack of write timeout?

Good question. Here is the scenario where we can run into problems in the DFSInputStream code:

DFSInputStream#getBlockReader uses BlockReaderFactory#build to create a new RemoteBlockReader2. Ultimately BlockReaderFactory#nextTcpPeer calls into DFSClient#newConnectedPeer. As you can see from this patch, this function sets a read timeout, but no write timeout.

I should also make an additional comment here: the Peer class we will be creating is NioInetPeer. You can see that NioInetPeer.java contains a org.apache.hadoop.net.SocketOutputStream inside it. This is the same class which DFSOutputStream is using to "simulate" socket write timeouts by using non-blocking I/O and selectors. But whereas DFSOutputStream uses org.apache.hadoop.net.SocketOutputStream directly, DFSInputStream uses it "under the covers" by using the Peer class. So the timeout mechanism is actually the same (Chris, I think this answers another question you raised.)

I always found it very confusing that org.apache.hadoop.net.SocketOutputStream is named the same as java.net.SocketOutputStream. Just for anyone new to Hadoop reading this-- they are very different classes!

So RemoteBlockReader2#newBlockReader has this code:

 
   // in and out will be closed when sock is closed (by the caller)
    final DataOutputStream out = new DataOutputStream(new BufferedOutputStream(
          peer.getOutputStream()));
    new Sender(out).readBlock(block, blockToken, clientName, startOffset, len,
        verifyChecksum, cachingStrategy);

    //
    // Get bytes in block
    //
    DataInputStream in = new DataInputStream(peer.getInputStream());

    BlockOpResponseProto status = BlockOpResponseProto.parseFrom(
        PBHelper.vintPrefixed(in));
    checkSuccess(status, peer, block, file);
    ReadOpChecksumInfoProto checksumInfo =
      status.getReadOpChecksumInfo();

You can see that there is a potential for things to get stuck here since we have not set a write timeout. That's the case this patch fixes. We're seeing this in production now so it's definitely more than a theoretical problem.

I am +1 on the patch. Chris Nauroth, can you take another look and verify that this answers your questions? We will hold off on committing until that point.

Ah, of course! This is not "block write timeout". This is "socket write timeout during block read operations". Thanks for the detailed follow-up.

+1 for the patch. Thank you, Xiaoyu, Esteban and Colin.

committed to 2.8, thanks all

FAILURE: Integrated in Hadoop-trunk-Commit #8162 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8162/)
HDFS-7608: hdfs dfsclient newConnectedPeer has no write timeout (Xiaoyu Yao via Colin P. McCabe) (cmccabe: rev 1d74ccececaefffaa90c0c18b40a3645dbc819d9)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
  HDFS-7608: add CHANGES.txt (cmccabe: rev b7fb6ec4513de7d342c541eb3d9e14642286e2cf)
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #257 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/257/)
HDFS-7608: hdfs dfsclient newConnectedPeer has no write timeout (Xiaoyu Yao via Colin P. McCabe) (cmccabe: rev 1d74ccececaefffaa90c0c18b40a3645dbc819d9)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
  HDFS-7608: add CHANGES.txt (cmccabe: rev b7fb6ec4513de7d342c541eb3d9e14642286e2cf)
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Yarn-trunk #987 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/987/)
HDFS-7608: hdfs dfsclient newConnectedPeer has no write timeout (Xiaoyu Yao via Colin P. McCabe) (cmccabe: rev 1d74ccececaefffaa90c0c18b40a3645dbc819d9)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
  HDFS-7608: add CHANGES.txt (cmccabe: rev b7fb6ec4513de7d342c541eb3d9e14642286e2cf)
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk #2184 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2184/)
HDFS-7608: hdfs dfsclient newConnectedPeer has no write timeout (Xiaoyu Yao via Colin P. McCabe) (cmccabe: rev 1d74ccececaefffaa90c0c18b40a3645dbc819d9)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
  HDFS-7608: add CHANGES.txt (cmccabe: rev b7fb6ec4513de7d342c541eb3d9e14642286e2cf)
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #245 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/245/)
HDFS-7608: hdfs dfsclient newConnectedPeer has no write timeout (Xiaoyu Yao via Colin P. McCabe) (cmccabe: rev 1d74ccececaefffaa90c0c18b40a3645dbc819d9)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
  HDFS-7608: add CHANGES.txt (cmccabe: rev b7fb6ec4513de7d342c541eb3d9e14642286e2cf)
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

SUCCESS: Integrated in Hadoop-Mapreduce-trunk-Java8 #255 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/255/)
HDFS-7608: hdfs dfsclient newConnectedPeer has no write timeout (Xiaoyu Yao via Colin P. McCabe) (cmccabe: rev 1d74ccececaefffaa90c0c18b40a3645dbc819d9)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
  HDFS-7608: add CHANGES.txt (cmccabe: rev b7fb6ec4513de7d342c541eb3d9e14642286e2cf)
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2203 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2203/)
HDFS-7608: hdfs dfsclient newConnectedPeer has no write timeout (Xiaoyu Yao via Colin P. McCabe) (cmccabe: rev 1d74ccececaefffaa90c0c18b40a3645dbc819d9)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
  HDFS-7608: add CHANGES.txt (cmccabe: rev b7fb6ec4513de7d342c541eb3d9e14642286e2cf)
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

can we get this backported to 2.6 and 2.7 please?