-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12646747/HDFS-6448.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6974//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6974//console

This message is automatically generated.

Liang Xie Should we just disable ping instead? Then the timeout would be ping interval (IPC_PING_INTERVAL_DEFAULT = 60000; // 1 min)

Yeh, here we have two fix chooses. seems replacing with another good default timeout(like socketTimeout) is more low-risk.
since disabling ping:
1) Client.getTimeout(conf) is called by NameNodeProxies.createNNProxyWithClientProtocol() as well, i am not 100% confident the getTimeout() result changing weather interfere this path or not so far.
2) what's the exact "ping" background be added, i don't know clearly.

Liang Xie Makes sense then to leave ping alone (The ping thing I am unclear on myself – It is purged from hbase RPC).

Patch lgtm then. Socket timeout looks more reasonable than the cryptic Client.getTimeout.

You running old-school short-circuit reads (HDFS-2246) then? Do you know if this is is only an issue in HDFS-2246 SCR? Is it present in HDFS-347 SCRs?

Thank Liang Xie

yes, we employed hadoop2.0, only the legacy HDFS-2246 available. I took a quick look at the HDFS-347 SCR code while making patch and did not find the same issue(to be honest, i am not familiar with this piece of code, so probably just i missed it). i think Colin P. McCabe have the exact answer definitely

Socket timeout seems reasonable to me. DFSInputStream uses socketTimeout to get a proxy to talk to the DN, in code like this:

  /** Read the block length from one of the datanodes. */
  private long readBlockLength(LocatedBlock locatedblock) throws IOException {
...
      try {
        cdp = DFSUtil.createClientDatanodeProtocolProxy(datanode,
            dfsClient.getConfiguration(), dfsClient.getConf().socketTimeout,
            dfsClient.getConf().connectToDnViaHostname, locatedblock);

So I am +1 on this patch.

yes, we employed hadoop2.0, only the legacy HDFS-2246 available. I took a quick look at the HDFS-347 SCR code while making patch and did not find the same issue(to be honest, i am not familiar with this piece of code, so probably just i missed it). i think Colin Patrick McCabe have the exact answer definitely

Just as a note, we kept around the legacy block reader local only because HDFS-347 wasn't implemented on Windows. If you are not using Windows, then I would recommend upgrading and using the new one ASAP... HDFS-2246 has a lot of problems besides this (its failure handling code is fairly buggy, especially in older releases.)

Do you know if this is is only an issue in HDFS-2246 SCR? Is it present in HDFS-347 SCRs?

HDFS-347 uses socketTimeout. The relevant code is in BlockReaderFactory#nextDomainPeer

Thanks Colin for your comment, now i begin to understand why the BlockReaderLocalLegacy class still in trunk and also i am glad to see this timeout issue doesn't exist in HDFS-347 SCR.

+1. Will commit today or tomorrow unless there's more to say here

+1

SUCCESS: Integrated in Hadoop-trunk-Commit #5616 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5616/)
HDFS-6448. BlockReaderLocalLegacy should set socket timeout based on conf.socketTimeout (liangxie via cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1598079)

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java

FAILURE: Integrated in Hadoop-Yarn-trunk #567 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/567/)
HDFS-6448. BlockReaderLocalLegacy should set socket timeout based on conf.socketTimeout (liangxie via cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1598079)

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java

FAILURE: Integrated in Hadoop-Hdfs-trunk #1758 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1758/)
HDFS-6448. BlockReaderLocalLegacy should set socket timeout based on conf.socketTimeout (liangxie via cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1598079)

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk #1786 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1786/)
HDFS-6448. BlockReaderLocalLegacy should set socket timeout based on conf.socketTimeout (liangxie via cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1598079)

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java