Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1573

LeaseChecker thread name trace not that useful

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: hdfs-client
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      The LeaseChecker thread in DFSClient will put a stack trace in its thread name, theoretically to help debug cases where these threads get leaked. However it just shows the stack trace of whoever is asking for the thread's name, not the stack trace of when the thread was allocated. I'd like to fix this so that you can see where the thread got started, which was presumably its original intent.

      1. hdfs-1573.txt
        12 kB
        Todd Lipcon
      2. hdfs-1573.txt
        12 kB
        Todd Lipcon
      3. hdfs-1573.txt
        12 kB
        Todd Lipcon
      4. hdfs-1573.txt
        12 kB
        Todd Lipcon

        Activity

        Hide
        Tsz Wo Nicholas Sze added a comment -

        Is this already fixed by now?

        Show
        Tsz Wo Nicholas Sze added a comment - Is this already fixed by now?
        Hide
        Todd Lipcon added a comment -

        I don't think so. I had a patch around somewhere, I'll see if I can find it and upload it (apparently forgot to when I made the JIRA)

        Show
        Todd Lipcon added a comment - I don't think so. I had a patch around somewhere, I'll see if I can find it and upload it (apparently forgot to when I made the JIRA)
        Hide
        Todd Lipcon added a comment -

        LeaseChecker is now called LeaseRenewer, but now the thread name is even less descriptive. It should contain the client name, the NN, and the UGI.

        Show
        Todd Lipcon added a comment - LeaseChecker is now called LeaseRenewer, but now the thread name is even less descriptive. It should contain the client name, the NN, and the UGI.
        Hide
        Todd Lipcon added a comment -

        Added some unit tests for LeaseRenewer, and fixed up the thread name.

        Show
        Todd Lipcon added a comment - Added some unit tests for LeaseRenewer, and fixed up the thread name.
        Hide
        Eli Collins added a comment -

        Why use an empty exception to record the time the renewal thread was started instead of a timestamp? Otherwise the patch looks great.

        Show
        Eli Collins added a comment - Why use an empty exception to record the time the renewal thread was started instead of a timestamp? Otherwise the patch looks great.
        Hide
        Todd Lipcon added a comment -

        The empty exception gets its stack trace filled in when it's instantiated. Then when we print it out later, we can see the call stack that caused the thread to get started - useful if trying to track down a leak.

        Show
        Todd Lipcon added a comment - The empty exception gets its stack trace filled in when it's instantiated. Then when we print it out later, we can see the call stack that caused the thread to get started - useful if trying to track down a leak.
        Hide
        Eli Collins added a comment -

        Makes sense, thanks.

        +1

        Show
        Eli Collins added a comment - Makes sense, thanks. +1
        Hide
        Tsz Wo Nicholas Sze added a comment -
        • Why using com.google.common.annotations.VisibleForTesting?
        • There are some not standard charcters
          -      return s + ", clients=" +  clientsString() + ", "
          -             + StringUtils.stringifyException(new Throwable("for testing"));
          +      return s + ", clients=" +  clientsString()
          +        + ", сreated at " +
          +        StringUtils.stringifyException(instantiationTrace);
          
        Show
        Tsz Wo Nicholas Sze added a comment - Why using com.google.common.annotations.VisibleForTesting ? There are some not standard charcters - return s + ", clients=" + clientsString() + ", " - + StringUtils.stringifyException( new Throwable( " for testing" )); + return s + ", clients=" + clientsString() + + ", сreated at " + + StringUtils.stringifyException(instantiationTrace);
        Hide
        Tsz Wo Nicholas Sze added a comment -
        +  int getHdfsTimeout() {
        +    return hdfsTimeout;
        +  }
        +  
        +  String getClientName() {
        +    return clientName;
        +  }
        

        hdfsTimeout and clientName are final constants. So these two methods are not needed.

        Show
        Tsz Wo Nicholas Sze added a comment - + int getHdfsTimeout() { + return hdfsTimeout; + } + + String getClientName() { + return clientName; + } hdfsTimeout and clientName are final constants. So these two methods are not needed.
        Hide
        Todd Lipcon added a comment -

        Woops, not sure how that Cyrillic 'с' got in there instead of a Latin 'c' (they look the same in most fonts).

        Why using com.google.common.annotations.VisibleForTesting?

        Since we have Guava around now, why not? It's a nice annotation to say that the function is only meant to be used by test code.

        hdfsTimeout and clientName are final constants. So these two methods are not needed

        Those are needed so that DFSClient can be mocked in the unit tests. It's not possible to stub a field, so we need it to be accessed through an accessor. I can mark these as @VisibleForTesting as well?

        Show
        Todd Lipcon added a comment - Woops, not sure how that Cyrillic 'с' got in there instead of a Latin 'c' (they look the same in most fonts). Why using com.google.common.annotations.VisibleForTesting? Since we have Guava around now, why not? It's a nice annotation to say that the function is only meant to be used by test code. hdfsTimeout and clientName are final constants. So these two methods are not needed Those are needed so that DFSClient can be mocked in the unit tests. It's not possible to stub a field, so we need it to be accessed through an accessor. I can mark these as @VisibleForTesting as well?
        Hide
        Todd Lipcon added a comment -

        Updated patch with no random UTF-8 characters

        Show
        Todd Lipcon added a comment - Updated patch with no random UTF-8 characters
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > Since we have Guava around now, why not? It's a nice annotation to say that the function is only meant to be used by test code.

        Is the annotation currently used in our test framework? There are various annotation packages. Why choose Guava?

        I like the idea on adding annotations if we are actually using them. Otherwise, they are just some unused codes.

        Show
        Tsz Wo Nicholas Sze added a comment - > Since we have Guava around now, why not? It's a nice annotation to say that the function is only meant to be used by test code. Is the annotation currently used in our test framework? There are various annotation packages. Why choose Guava? I like the idea on adding annotations if we are actually using them. Otherwise, they are just some unused codes.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > Those are needed so that DFSClient can be mocked in the unit tests. ...

        This is a good point. Thanks, Todd.

        Show
        Tsz Wo Nicholas Sze added a comment - > Those are needed so that DFSClient can be mocked in the unit tests. ... This is a good point. Thanks, Todd.
        Hide
        Todd Lipcon added a comment -

        Is the annotation currently used in our test framework? There are various annotation packages. Why choose Guava?

        Guava was already included in Hadoop Common since HADOOP-6919. I opened a thread on the mailing list a couple months ago and received several +1s to the effect that it's a useful dependency. Keep in mind that Guava provides a lot of useful stuff - @VisibleForTesting is just one little thing in there.

        We don't currently do anything useful with the annotation, but it's at least as good as a comment that says /** Visible for testing */ - but better since down the road we could use it to exclude JavaDoc generation, for example. If you're strongly against it, happy to remove.

        Show
        Todd Lipcon added a comment - Is the annotation currently used in our test framework? There are various annotation packages. Why choose Guava? Guava was already included in Hadoop Common since HADOOP-6919 . I opened a thread on the mailing list a couple months ago and received several +1s to the effect that it's a useful dependency. Keep in mind that Guava provides a lot of useful stuff - @VisibleForTesting is just one little thing in there. We don't currently do anything useful with the annotation, but it's at least as good as a comment that says /** Visible for testing */ - but better since down the road we could use it to exclude JavaDoc generation, for example. If you're strongly against it, happy to remove.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > ... If you're strongly against it, happy to remove.

        Since @VisibleForTesting is not yet used in our test framework, I think it is rather confusing to use it here. Please remove it. We could add such annotations when we are actually using them.

        • For the getDaemon() method, how about change it to getDaemonName() which returns a string?
        • For instantiationTrace, it is better to create it using new Throwable("TRACE") and store it as a string.
        Show
        Tsz Wo Nicholas Sze added a comment - > ... If you're strongly against it, happy to remove. Since @VisibleForTesting is not yet used in our test framework, I think it is rather confusing to use it here. Please remove it. We could add such annotations when we are actually using them. For the getDaemon() method, how about change it to getDaemonName() which returns a string? For instantiationTrace , it is better to create it using new Throwable("TRACE") and store it as a string.
        Hide
        Todd Lipcon added a comment -

        This patch addresses Nicholas's latest comments.

        Show
        Todd Lipcon added a comment - This patch addresses Nicholas's latest comments.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        We should use junit 4.

        +import junit.framework.Assert;
        

        Otherwise, patch looks good.

        Show
        Tsz Wo Nicholas Sze added a comment - We should use junit 4. + import junit.framework.Assert; Otherwise, patch looks good.
        Hide
        Todd Lipcon added a comment -

        Woops, bad Eclipse! Fixed to junit 4 Assert class.

        Show
        Todd Lipcon added a comment - Woops, bad Eclipse! Fixed to junit 4 Assert class.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1
        Thanks Todd.

        Show
        Tsz Wo Nicholas Sze added a comment - +1 Thanks Todd.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12479402/hdfs-1573.txt
        against trunk revision 1103987.

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

        +1 tests included. The patch appears to include 2 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

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

        +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 failed these core unit tests:
        org.apache.hadoop.hdfs.server.namenode.TestBlocksWithNotEnoughRacks
        org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
        org.apache.hadoop.hdfs.TestFileConcurrentReader
        org.apache.hadoop.tools.TestJMXGet

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/537//testReport/
        Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/537//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/537//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/12479402/hdfs-1573.txt against trunk revision 1103987. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +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 failed these core unit tests: org.apache.hadoop.hdfs.server.namenode.TestBlocksWithNotEnoughRacks org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader org.apache.hadoop.tools.TestJMXGet +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/537//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/537//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/537//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        The failing tests are intermittent and/or known to be failing (reopened HDFS-1828 for the TestBlocksWithNotEnoughRacks failure)

        I'll commit this momentarily

        Show
        Todd Lipcon added a comment - The failing tests are intermittent and/or known to be failing (reopened HDFS-1828 for the TestBlocksWithNotEnoughRacks failure) I'll commit this momentarily
        Hide
        Todd Lipcon added a comment -

        Committed to trunk. Thanks for the reviews, Nicholas!

        Show
        Todd Lipcon added a comment - Committed to trunk. Thanks for the reviews, Nicholas!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #662 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/662/)
        HDFS-1573. Add useful tracing information to Lease Renewer thread names. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1104526
        Files :

        • /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestLeaseRenewer.java
        • /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/LeaseRenewer.java
        • /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
        • /hadoop/hdfs/trunk/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #662 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/662/ ) HDFS-1573 . Add useful tracing information to Lease Renewer thread names. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1104526 Files : /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestLeaseRenewer.java /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/LeaseRenewer.java /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/hdfs/trunk/CHANGES.txt
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #673 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/673/)

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

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development