Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.22.0, 0.23.0
    • Fix Version/s: 0.22.0, 0.23.0
    • Component/s: test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Will attach the full log. Here's the relevant snippet:

      Exception in thread "FSImageSaver for /home/eli/src/hdfs4/build/test/data/dfs/na
      me1 of type IMAGE_AND_EDITS" java.lang.RuntimeException: Injected fault: saveFSI
      mage second time
      ....
              at java.lang.Thread.run(Thread.java:619)
      Exception in thread "FSImageSaver for /home/eli/src/hdfs4/build/test/data/dfs/na
      me2 of type IMAGE_AND_EDITS" java.lang.StackOverflowError
              at java.util.Arrays.copyOf(Arrays.java:2882)
      
      1. hdfs-1503_0.22.patch
        1 kB
        Konstantin Boudnik
      2. hdfs-1503.txt
        1 kB
        Todd Lipcon
      3. TEST-org.apache.hadoop.hdfs.server.namenode.TestSaveNamespace.txt
        817 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          Full log attached.

          Show
          Eli Collins added a comment - Full log attached.
          Hide
          Todd Lipcon added a comment -

          This is a regression caused by HDFS-1071

          Show
          Todd Lipcon added a comment - This is a regression caused by HDFS-1071
          Hide
          Todd Lipcon added a comment -

          edited issue summary - this isn't from one of the AOP-based FI tests - it's just fault injection using Mockito from a normal unit test

          Show
          Todd Lipcon added a comment - edited issue summary - this isn't from one of the AOP-based FI tests - it's just fault injection using Mockito from a normal unit test
          Hide
          Todd Lipcon added a comment -

          Seems the first problem is that the faulty image delegates back to itself rather than the original image, so you get a stack overflow. After fixing that, the test still fails.

          Show
          Todd Lipcon added a comment - Seems the first problem is that the faulty image delegates back to itself rather than the original image, so you get a stack overflow. After fixing that, the test still fails.
          Hide
          Todd Lipcon added a comment -

          This patch fixes it for me.

          Show
          Todd Lipcon added a comment - This patch fixes it for me.
          Hide
          Konstantin Boudnik added a comment -

          +1 patch looks good.

          Show
          Konstantin Boudnik added a comment - +1 patch looks good.
          Hide
          Konstantin Shvachko added a comment -

          Could anybody please explain the problem and the solution. A don't understand it from the comments above.

          Show
          Konstantin Shvachko added a comment - Could anybody please explain the problem and the solution. A don't understand it from the comments above.
          Hide
          Todd Lipcon added a comment -

          Sure, no problem. This test uses a mockito "spy" object to inject some code right before the saveFSImage call. In some cases we want to call through to the original saveFSImage, in other cases we just want to throw an exception. For whatever reason, an incorrect change was made in HDFS-1071 which made the "call through" case call through to the same spy object rather than the unmodified class, so of course it infinitely recursed and blew out the stack. The patch switches to using the more "proper" way of doing this in Mockito - the callRealMethod() function.

          Show
          Todd Lipcon added a comment - Sure, no problem. This test uses a mockito "spy" object to inject some code right before the saveFSImage call. In some cases we want to call through to the original saveFSImage, in other cases we just want to throw an exception. For whatever reason, an incorrect change was made in HDFS-1071 which made the "call through" case call through to the same spy object rather than the unmodified class, so of course it infinitely recursed and blew out the stack. The patch switches to using the more "proper" way of doing this in Mockito - the callRealMethod() function.
          Hide
          Konstantin Shvachko added a comment -

          Thanks for clarifications, Todd. I was not sure if the new code broke the test itself or the feature it is testing. Now I understand it the test problem.

          Show
          Konstantin Shvachko added a comment - Thanks for clarifications, Todd. I was not sure if the new code broke the test itself or the feature it is testing. Now I understand it the test problem.
          Hide
          Konstantin Shvachko added a comment -

          +1. Tested on my box: testCrashWhileSavingSecondImage() fails without the patch, but passes with it.

          Show
          Konstantin Shvachko added a comment - +1. Tested on my box: testCrashWhileSavingSecondImage() fails without the patch, but passes with it.
          Hide
          Todd Lipcon added a comment -

          anyone want to commit?

          Show
          Todd Lipcon added a comment - anyone want to commit?
          Hide
          Eli Collins added a comment -
               [exec] 
               [exec] +1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     +1 tests included.  The patch appears to include 3 new or modified tests.
               [exec] 
               [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
               [exec] 
               [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
               [exec] 
               [exec]     +1 findbugs.  The patch does not introduce any new Findbugs (version 1.3.9) warnings.
               [exec] 
               [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
               [exec] 
               [exec]     +1 system test framework.  The patch passed system test framework compile.
               [exec] 
          
          Show
          Eli Collins added a comment - [exec] [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] +1 system test framework. The patch passed system test framework compile. [exec]
          Hide
          Konstantin Boudnik added a comment -

          I have just committed this to trunk. Thanks Todd!

          The patch doesn't apply to 0.22. Moreover, in 0.22 this test passes on my machine. Please let me know if this patch (with minor variation) is applicable for 0.22 as well.

          Show
          Konstantin Boudnik added a comment - I have just committed this to trunk. Thanks Todd! The patch doesn't apply to 0.22. Moreover, in 0.22 this test passes on my machine. Please let me know if this patch (with minor variation) is applicable for 0.22 as well.
          Hide
          Konstantin Boudnik added a comment -

          Just in case the modification of the patch for 0.22 in case it needs to be committed as well.

          Show
          Konstantin Boudnik added a comment - Just in case the modification of the patch for 0.22 in case it needs to be committed as well.
          Hide
          Eli Collins added a comment -

          Hey Cos,

          Looks like we were working at the same time, I already backported it to 22 (which is why you probably saw that the patch didn't apply).

          Thanks,
          Eli

          Show
          Eli Collins added a comment - Hey Cos, Looks like we were working at the same time, I already backported it to 22 (which is why you probably saw that the patch didn't apply). Thanks, Eli
          Hide
          Konstantin Boudnik added a comment -

          Please disregard my last comment about 0.22 - looks like Eli has merged the commit to 0.22 almost concurrently, although it seemed unnecessary.

          Show
          Konstantin Boudnik added a comment - Please disregard my last comment about 0.22 - looks like Eli has merged the commit to 0.22 almost concurrently, although it seemed unnecessary.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-22-branch #35 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/35/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-22-branch #35 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/35/ )

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development