Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-167

DFSClient continues to retry indefinitely

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.1, 0.21.0
    • Component/s: hdfs-client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      I encountered a bug when trying to upload data using the Hadoop DFS Client.
      After receiving a NotReplicatedYetException, the DFSClient will normally retry its upload up to some limited number of times. In this case, I found that this retry loop continued indefinitely, to the point that the number of tries remaining was negative:
      2009-03-25 16:20:02 [INFO]
      2009-03-25 16:20:02 [INFO] 09/03/25 16:20:02 INFO hdfs.DFSClient: Waiting for replication for 21 seconds
      2009-03-25 16:20:03 [INFO] 09/03/25 16:20:02 WARN hdfs.DFSClient: NotReplicatedYetException sleeping /apollo/env/SummaryMySQL/var/logstore/fiorello_logs_2009
      0325_us/logs_20090325_us_13 retries left -1

      The stack trace for the failure that's retrying is:
      2009-03-25 16:20:02 [INFO] 09/03/25 16:20:02 INFO hdfs.DFSClient: org.apache.hadoop.ipc.RemoteException: org.apache.hadoop.hdfs.server.namenode.NotReplicated
      YetException: Not replicated yet:<filename>
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.getAdditionalBlock(FSNamesystem.java:1266)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.hdfs.server.namenode.NameNode.addBlock(NameNode.java:351)
      2009-03-25 16:20:02 [INFO] at sun.reflect.GeneratedMethodAccessor19.invoke(Unknown Source)
      2009-03-25 16:20:02 [INFO] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      2009-03-25 16:20:02 [INFO] at java.lang.reflect.Method.invoke(Method.java:597)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:481)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.ipc.Server$Handler.run(Server.java:894)
      2009-03-25 16:20:02 [INFO]
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.ipc.Client.call(Client.java:697)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.ipc.RPC$Invoker.invoke(RPC.java:216)
      2009-03-25 16:20:02 [INFO] at $Proxy0.addBlock(Unknown Source)
      2009-03-25 16:20:02 [INFO] at sun.reflect.GeneratedMethodAccessor2.invoke(Unknown Source)
      2009-03-25 16:20:02 [INFO] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      2009-03-25 16:20:02 [INFO] at java.lang.reflect.Method.invoke(Method.java:597)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.io.retry.RetryInvocationHandler.invokeMethod(RetryInvocationHandler.java:82)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:59)
      2009-03-25 16:20:02 [INFO] at $Proxy0.addBlock(Unknown Source)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.locateFollowingBlock(DFSClient.java:2814)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.nextBlockOutputStream(DFSClient.java:2696)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.access$2000(DFSClient.java:1996)
      2009-03-25 16:20:02 [INFO] at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream$DataStreamer.run(DFSClient.java:2183)

      Fixes logical error in DFSClient::DFSOutputStream::DataStreamer::locateFollowingBlock that caused infinite retries on write. Modified DFSClient constructor to allow unit testing of locateFollowingBlock and added unit tests.

      1. hdfs-167-for-20-1.patch
        11 kB
        Bill Zeller
      2. hdfs-167-6.patch
        11 kB
        Bill Zeller
      3. hdfs-167-5.patch
        35 kB
        Bill Zeller
      4. hdfs-167-4.patch
        12 kB
        Bill Zeller

        Issue Links

          Activity

          Hide
          Hairong Kuang added a comment -

          Although the negative retry number is misleading, it seems to me retrying forever when allocating a new block is a policy when a previous block in a file being created does not meet the minimum replication factor. DFS does the same for close. Close retries forever if any of blocks in the file does not meet the minimum replication factor. Retrying forever may not be a good policy. A dfs client should retry a finite times and then declare a failure to the client.

          Show
          Hairong Kuang added a comment - Although the negative retry number is misleading, it seems to me retrying forever when allocating a new block is a policy when a previous block in a file being created does not meet the minimum replication factor. DFS does the same for close. Close retries forever if any of blocks in the file does not meet the minimum replication factor. Retrying forever may not be a good policy. A dfs client should retry a finite times and then declare a failure to the client.
          Hide
          dhruba borthakur added a comment -

          The are potions of the client code that retries indefinitely. Especially when the file is closed. II agree completely that this portion of the client code should be changed to retry for a large finite amount of time.

          Show
          dhruba borthakur added a comment - The are potions of the client code that retries indefinitely. Especially when the file is closed. II agree completely that this portion of the client code should be changed to retry for a large finite amount of time.
          Hide
          Bill Zeller added a comment -

          The offending code:

          if (--retries == 0 &&
          !NotReplicatedYetException.class.getName().
          equals(e.getClassName()))

          Unknown macro: { throw e; }

          This code attempts to retry until the above condition is met. The above condition says to throw e if the number of retries is 0 and the exception thrown is not a NotReplicatedYetException. However, the code later assumes that any exception not thrown is a NotReplicatedYetException. The intent seems to be to retry a certain number of times if a NotReplicatedYetException is thrown and to throw any other type of exception. The && in the if statement should be changed to an ||.

          Show
          Bill Zeller added a comment - The offending code: if (--retries == 0 && !NotReplicatedYetException.class.getName(). equals(e.getClassName())) Unknown macro: { throw e; } This code attempts to retry until the above condition is met. The above condition says to throw e if the number of retries is 0 and the exception thrown is not a NotReplicatedYetException . However, the code later assumes that any exception not thrown is a NotReplicatedYetException . The intent seems to be to retry a certain number of times if a NotReplicatedYetException is thrown and to throw any other type of exception. The && in the if statement should be changed to an || .
          Hide
          Bill Zeller added a comment -

          The above code should be:

          org.apache.hadoop.hdfs.DFSClient::locateFollowingBlock
          if (--retries == 0 && 
              !NotReplicatedYetException.class.getName().
              equals(e.getClassName())) {
              throw e;
          }
          

          (Sorry about the repost)

          Show
          Bill Zeller added a comment - The above code should be: org.apache.hadoop.hdfs.DFSClient::locateFollowingBlock if (--retries == 0 && !NotReplicatedYetException.class.getName(). equals(e.getClassName())) { throw e; } (Sorry about the repost)
          Hide
          dhruba borthakur added a comment -

          Hi Bill, will it be possible for you to submit this as a patch and a unit test? Details are here : http://wiki.apache.org/hadoop/HowToContribute

          Show
          dhruba borthakur added a comment - Hi Bill, will it be possible for you to submit this as a patch and a unit test? Details are here : http://wiki.apache.org/hadoop/HowToContribute
          Hide
          Bill Zeller added a comment -

          Hi--Yes, I'll be submitting a patch for this. This issue is currently unassigned because I don't have permission to assign it to myself. There appears to be no easy way to test this through unit tests (without actually starting a namenode and datanode), so I'd like to modify the class to make direct unit testing easier.

          Show
          Bill Zeller added a comment - Hi--Yes, I'll be submitting a patch for this. This issue is currently unassigned because I don't have permission to assign it to myself. There appears to be no easy way to test this through unit tests (without actually starting a namenode and datanode), so I'd like to modify the class to make direct unit testing easier.
          Hide
          Jakob Homan added a comment -

          Reviewed patch. Overall looks good.

          • The new constructor may be confusing - an inet addr and the namenode objects? Rather than the if statement in the new constructor, it may be better to have two constructors, one that takes an Inet addr, and one that takes the NNs directly. To avoid duplicating code, it would be necessary to not make the ClientProtocol instances final. I tried playing around with nested private/public constructors to avoid the code duplication and was surprised I couldn't get away with it.
          • Is there any other use case for the new constructor other than testing? If not, it may be good to note the ctor is used for testing in the comments.
          • We try to stay within the 80 column limit generally, even if it means weird wrapping.
          • There is commented-out code in TestDFSClientRertries import section that should be removed.
          • I like the mock namenode. For its empty methods, it would be better to either add a comment in each that it's not needed, or to group all together with a preceding comment to that effect and provide really empty bodies {} (perhaps on the same line). Better for readability and gets the point across more clearly. Another option would be to throw unsupportedOperationExcetion, but that's probably overkill.
          • It's better to use this format for the logger:
            public static final Log LOG = LogFactory.getLog(CLASS.class.getName());

            rather than specifying the clas name in a string, so that if the class is refactored or changed, the logging statement doesn't need to be updated.

          Show
          Jakob Homan added a comment - Reviewed patch. Overall looks good. The new constructor may be confusing - an inet addr and the namenode objects? Rather than the if statement in the new constructor, it may be better to have two constructors, one that takes an Inet addr, and one that takes the NNs directly. To avoid duplicating code, it would be necessary to not make the ClientProtocol instances final. I tried playing around with nested private/public constructors to avoid the code duplication and was surprised I couldn't get away with it. Is there any other use case for the new constructor other than testing? If not, it may be good to note the ctor is used for testing in the comments. We try to stay within the 80 column limit generally, even if it means weird wrapping. There is commented-out code in TestDFSClientRertries import section that should be removed. I like the mock namenode. For its empty methods, it would be better to either add a comment in each that it's not needed, or to group all together with a preceding comment to that effect and provide really empty bodies {} (perhaps on the same line). Better for readability and gets the point across more clearly. Another option would be to throw unsupportedOperationExcetion, but that's probably overkill. It's better to use this format for the logger: public static final Log LOG = LogFactory.getLog(CLASS.class.getName()); rather than specifying the clas name in a string, so that if the class is refactored or changed, the logging statement doesn't need to be updated.
          Hide
          Jakob Homan added a comment -

          Talked with Bill and he points out the Namenode field in question is public, which is a mistake and should be fixed with this patch, since we're in that neck of the code woods anyway.

          Show
          Jakob Homan added a comment - Talked with Bill and he points out the Namenode field in question is public, which is a mistake and should be fixed with this patch, since we're in that neck of the code woods anyway.
          Hide
          Bill Zeller added a comment -

          I addressed the six bullet points in Jakob's comment above and also made the namenode field private in DFSClient. This required changing referencing code, which is why the new patch is so much larger than the previous one.

          Show
          Bill Zeller added a comment - I addressed the six bullet points in Jakob's comment above and also made the namenode field private in DFSClient. This required changing referencing code, which is why the new patch is so much larger than the previous one.
          Hide
          dhruba borthakur added a comment -

          It would be really nice if we can split this into two patches: one patch that fixes the infinite loop and the other is a code-cleanup related to making dfs.namenode private. The reason being that he infinite loop problem can be used by people to backport to previous hadoop releases, especially hadoop 0.20.

          Show
          dhruba borthakur added a comment - It would be really nice if we can split this into two patches: one patch that fixes the infinite loop and the other is a code-cleanup related to making dfs.namenode private. The reason being that he infinite loop problem can be used by people to backport to previous hadoop releases, especially hadoop 0.20.
          Hide
          Jakob Homan added a comment -

          The code-cleanup part wasn't really a code-cleanup. With the new ctor introduced, Bill had to make the namenode instance nonfinal. This introduces the possibility of it being changed directly, since it's a public field, which would be really bad. The change wasn't cosmetic or optional; it was a logical thing to do what with the new ctor. Is there any reason why that portion couldn't be backported as well?

          Show
          Jakob Homan added a comment - The code-cleanup part wasn't really a code-cleanup. With the new ctor introduced, Bill had to make the namenode instance nonfinal. This introduces the possibility of it being changed directly, since it's a public field, which would be really bad. The change wasn't cosmetic or optional; it was a logical thing to do what with the new ctor. Is there any reason why that portion couldn't be backported as well?
          Hide
          Bill Zeller added a comment -

          Unit testing the DFSClient infinite retry fix requires creating a DFSClient object with a mock namenode object. In order to add a ctor to DFSClient without duplicating code that accepts a mock object, DFSClient.namenode needs to be made nonfinal. This cannot be safely done because DFSClient.namenode is public. HDFS-514 makes the DFSClient.namenode field private and changes code that references that field to use a getter. This allows this patch to add a new ctor safely.

          Show
          Bill Zeller added a comment - Unit testing the DFSClient infinite retry fix requires creating a DFSClient object with a mock namenode object. In order to add a ctor to DFSClient without duplicating code that accepts a mock object, DFSClient.namenode needs to be made nonfinal. This cannot be safely done because DFSClient.namenode is public. HDFS-514 makes the DFSClient.namenode field private and changes code that references that field to use a getter. This allows this patch to add a new ctor safely.
          Hide
          dhruba borthakur added a comment -

          The change that changes DFSClient.namenode from public to private cannot be backported easily to previous releases becaase it changes a public API, isn't it?

          Show
          dhruba borthakur added a comment - The change that changes DFSClient.namenode from public to private cannot be backported easily to previous releases becaase it changes a public API, isn't it?
          Hide
          Jakob Homan added a comment -

          not really. The only code that uses the public NN (on trunk at least) is a jsp page and some tests.

          Show
          Jakob Homan added a comment - not really. The only code that uses the public NN (on trunk at least) is a jsp page and some tests.
          Hide
          Jakob Homan added a comment -

          and there is a getter method that still provides access to the field. Code just goes through the getter instead.

          Show
          Jakob Homan added a comment - and there is a getter method that still provides access to the field. Code just goes through the getter instead.
          Hide
          dhruba borthakur added a comment -

          It appears that you already made HDFS-514 to check in only the changes to DFSClient.namenode. Thanks.

          Show
          dhruba borthakur added a comment - It appears that you already made HDFS-514 to check in only the changes to DFSClient.namenode. Thanks.
          Hide
          Bill Zeller added a comment -

          Patch works with HDFS-514.

          ----------------------------
          test-patch (run locally):

          [exec] +1 overall.
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec] +1 tests included. The patch appears to include 3 new or modified tests.
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
          ======================================================================
          ======================================================================
          [exec] Finished build.
          ======================================================================
          ======================================================================

          BUILD SUCCESSFUL
          Total time: 10 minutes 8 seconds
          ----------------------------

          ----------------------------
          "ant test":

          test:

          BUILD SUCCESSFUL
          Total time: 39 minutes 53 seconds
          ----------------------------

          Show
          Bill Zeller added a comment - Patch works with HDFS-514 . ---------------------------- test-patch (run locally): [exec] +1 overall. [exec] +1 @author. The patch does not contain any @author tags. [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. ====================================================================== ====================================================================== [exec] Finished build. ====================================================================== ====================================================================== BUILD SUCCESSFUL Total time: 10 minutes 8 seconds ---------------------------- ---------------------------- "ant test": test: BUILD SUCCESSFUL Total time: 39 minutes 53 seconds ----------------------------
          Hide
          Jakob Homan added a comment -

          Patch looks good. +1.

          Show
          Jakob Homan added a comment - Patch looks good. +1.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ... infinite loop problem can be used by people to backport to previous hadoop releases, especially hadoop 0.20.

          Because this depends on HDFS-514, I suggest we only commit this to trunk. Of course, we should provide (but not commit) a patch for 0.20.

          Dhruba, how does it sound to you?

          Show
          Tsz Wo Nicholas Sze added a comment - > ... infinite loop problem can be used by people to backport to previous hadoop releases, especially hadoop 0.20. Because this depends on HDFS-514 , I suggest we only commit this to trunk. Of course, we should provide (but not commit) a patch for 0.20. Dhruba, how does it sound to you?
          Hide
          dhruba borthakur added a comment -

          if there is a patch for 0.20 attached to this JIRA, that will be awesome. if not, I can live with that too.

          Show
          dhruba borthakur added a comment - if there is a patch for 0.20 attached to this JIRA, that will be awesome. if not, I can live with that too.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Bill!

          Could you post a patch for 0.20?

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Bill! Could you post a patch for 0.20?
          Hide
          Bill Zeller added a comment -

          hdfs-167-for-20-1.patch is a patch for .20.

          Running test-patch:

          [exec] +1 overall.
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec] +1 tests included. The patch appears to include 3 new or modified tests.
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
          ======================================================================
          ======================================================================
          [exec] Finished build.
          ======================================================================
          ======================================================================

          Running "ant test" passed all test except one (org.apache.hadoop.cli.TestCLI) which I believe is a configuration error on my part (I get the same error when testing the trunk with no modifications).

          Show
          Bill Zeller added a comment - hdfs-167-for-20-1.patch is a patch for .20. Running test-patch: [exec] +1 overall. [exec] +1 @author. The patch does not contain any @author tags. [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. ====================================================================== ====================================================================== [exec] Finished build. ====================================================================== ====================================================================== Running "ant test" passed all test except one (org.apache.hadoop.cli.TestCLI) which I believe is a configuration error on my part (I get the same error when testing the trunk with no modifications).
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #36 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/36/)
          . Fix a bug in DFSClient that caused infinite retries on write. Contributed by Bill Zeller

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #36 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/36/ ) . Fix a bug in DFSClient that caused infinite retries on write. Contributed by Bill Zeller
          Hide
          dhruba borthakur added a comment -

          Thanks for the 0.20 patch. We added a new constructor to DFSClient:

          • This constructor was written to allow easy testing of the DFSClient class.
            + * End users will most likely want to use one of the other constructors.
            + */
            + public DFSClient(ClientProtocol namenode, ClientProtocol rpcNamenode,
            + Configuration conf, FileSystem.Statistics stats)

          is it possible to make this constructor package-private (instead of public)?

          Show
          dhruba borthakur added a comment - Thanks for the 0.20 patch. We added a new constructor to DFSClient: This constructor was written to allow easy testing of the DFSClient class. + * End users will most likely want to use one of the other constructors. + */ + public DFSClient(ClientProtocol namenode, ClientProtocol rpcNamenode, + Configuration conf, FileSystem.Statistics stats) is it possible to make this constructor package-private (instead of public)?
          Hide
          Bill Zeller added a comment -

          If an end-user has the appropriate ClientProtocol objects, I have no problem with him using this constructor. Is there a reason to restrict dependency injection?

          Show
          Bill Zeller added a comment - If an end-user has the appropriate ClientProtocol objects, I have no problem with him using this constructor. Is there a reason to restrict dependency injection?
          Hide
          Jakob Homan added a comment -

          I tend to agree with Bill. I don't see any harm in having the extra ctor. Moreover, since this patch has already been applied to trunk, the new change would only be introduced to 20, which seems like not the best idea.

          Show
          Jakob Homan added a comment - I tend to agree with Bill. I don't see any harm in having the extra ctor. Moreover, since this patch has already been applied to trunk, the new change would only be introduced to 20, which seems like not the best idea.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          DFSClient currently has 5 constructors. I think what we need is the most general constructor (the one with the most parameters) plus one or two constructors for convenient. Let's do it in a separated issue, HDFS-527.

          Show
          Tsz Wo Nicholas Sze added a comment - DFSClient currently has 5 constructors. I think what we need is the most general constructor (the one with the most parameters) plus one or two constructors for convenient. Let's do it in a separated issue, HDFS-527 .
          Hide
          Jakob Homan added a comment -

          +1 on the backport. I ran TestCLI locally with the patch and it passed.

          Show
          Jakob Homan added a comment - +1 on the backport. I ran TestCLI locally with the patch and it passed.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Reopen for committing to 0.20.

          Show
          Tsz Wo Nicholas Sze added a comment - Reopen for committing to 0.20.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this to 0.20. Will also commit HDFS-527 to 0.20.

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this to 0.20. Will also commit HDFS-527 to 0.20.
          Hide
          Robert Chansler added a comment -

          Editorial pass over all release notes prior to publication of 0.21.

          Show
          Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21.

            People

            • Assignee:
              Bill Zeller
              Reporter:
              Derek Wollenstein
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development