Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-6448

BlockReaderLocalLegacy should set socket timeout based on conf.socketTimeout

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 2.4.0
    • Fix Version/s: 2.5.0
    • Component/s: hdfs-client
    • Labels:
      None
    • Target Version/s:

      Description

      Our hbase deployed upon hadoop2.0, in one accident, we hit HDFS-5016 in HDFS side, but we also found from HBase side, the dfs client was hung at getBlockReader, after reading code, we found there is a timeout setting in current codebase though, but the default hdfsTimeout value is "-1" ( from Client.java:getTimeout(conf) )which means no timeout...

      The hung stack trace like following:
      at $Proxy21.getBlockLocalPathInfo(Unknown Source)
      at org.apache.hadoop.hdfs.protocolPB.ClientDatanodeProtocolTranslatorPB.getBlockLocalPathInfo(ClientDatanodeProtocolTranslatorPB.java:215)
      at org.apache.hadoop.hdfs.BlockReaderLocal.getBlockPathInfo(BlockReaderLocal.java:267)
      at org.apache.hadoop.hdfs.BlockReaderLocal.newBlockReader(BlockReaderLocal.java:180)
      at org.apache.hadoop.hdfs.DFSClient.getLocalBlockReader(DFSClient.java:812)

      One feasible fix is replacing the hdfsTimeout with socketTimeout. see attached patch. Most of credit should give Liu Shaohui

      1. HDFS-6448.txt
        0.7 kB
        Liang Xie

        Activity

        Hide
        Hudson added a comment -

        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
        Show
        Hudson added a comment - 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
        Hide
        Hudson added a comment -

        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
        Show
        Hudson added a comment - 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
        Hide
        Hudson added a comment -

        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
        Show
        Hudson added a comment - 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
        Hide
        Hudson added a comment -

        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
        Show
        Hudson added a comment - 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
        Hide
        stack added a comment -

        +1

        Show
        stack added a comment - +1
        Hide
        Colin Patrick McCabe added a comment -

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

        Show
        Colin Patrick McCabe added a comment - +1. Will commit today or tomorrow unless there's more to say here
        Hide
        Liang Xie added a comment -

        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.

        Show
        Liang Xie added a comment - 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.
        Hide
        Colin Patrick McCabe added a comment -

        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

        Show
        Colin Patrick McCabe added a comment - 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
        Hide
        Liang Xie added a comment -

        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

        Show
        Liang Xie added a comment - 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
        Hide
        stack added a comment -

        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

        Show
        stack added a comment - 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
        Hide
        Liang Xie added a comment -

        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.

        Show
        Liang Xie added a comment - 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.
        Hide
        stack added a comment -

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

        Show
        stack added a comment - Liang Xie Should we just disable ping instead? Then the timeout would be ping interval (IPC_PING_INTERVAL_DEFAULT = 60000; // 1 min)
        Hide
        Hadoop QA added a comment -

        -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.

        Show
        Hadoop QA added a comment - -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.

          People

          • Assignee:
            Liang Xie
            Reporter:
            Liang Xie
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development