Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.98.0, 0.99.0
    • Fix Version/s: 0.99.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      A sub-task of HBASE-10497

      1. correct wrong handling of InterruptedException where Thread.currentThread.interrupt() is called within while loops
      2. add proper handling for swallowed InterruptedException
      1. split.patch
        8 kB
        Nicolas Liochon
      2. HBASE-10524-trunk_v2.patch
        6 kB
        Honghua Feng
      3. HBASE-10524-trunk_v1.patch
        6 kB
        Honghua Feng

        Activity

        Hide
        Enis Soztutar added a comment -

        Closing this issue after 0.99.0 release.

        Show
        Enis Soztutar added a comment - Closing this issue after 0.99.0 release.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #95 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/95/)
        HBASE-10524 Correct wrong handling and add proper handling for swallowed InterruptedException thrown by Thread.sleep in regionserver (Feng Honghua) (nkeywal: rev 1570219)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #95 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/95/ ) HBASE-10524 Correct wrong handling and add proper handling for swallowed InterruptedException thrown by Thread.sleep in regionserver (Feng Honghua) (nkeywal: rev 1570219) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4937 (See https://builds.apache.org/job/HBase-TRUNK/4937/)
        HBASE-10524 Correct wrong handling and add proper handling for swallowed InterruptedException thrown by Thread.sleep in regionserver (Feng Honghua) (nkeywal: rev 1570219)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4937 (See https://builds.apache.org/job/HBase-TRUNK/4937/ ) HBASE-10524 Correct wrong handling and add proper handling for swallowed InterruptedException thrown by Thread.sleep in regionserver (Feng Honghua) (nkeywal: rev 1570219) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
        Hide
        Nicolas Liochon added a comment -

        Committed to trunk.

        Show
        Nicolas Liochon added a comment - Committed to trunk.
        Hide
        Honghua Feng added a comment -

        I ran unit tests with split.patch on locally and all tests passed

        Show
        Honghua Feng added a comment - I ran unit tests with split.patch on locally and all tests passed
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12629769/split.patch
        against trunk revision .
        ATTACHMENT ID: 12629769

        +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 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +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 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.master.TestDistributedLogSplitting

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//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/12629769/split.patch against trunk revision . ATTACHMENT ID: 12629769 +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 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.master.TestDistributedLogSplitting Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8745//console This message is automatically generated.
        Hide
        Nicolas Liochon added a comment -

        Thanks, so, I will commit the two parts if nobody disagrees.

        Show
        Nicolas Liochon added a comment - Thanks, so, I will commit the two parts if nobody disagrees.
        Hide
        Honghua Feng added a comment -

        btw: the attached patch doesn't contain change to HRegionServer? please remember to include that when committing, or let me make a new one?

        Show
        Honghua Feng added a comment - btw: the attached patch doesn't contain change to HRegionServer? please remember to include that when committing, or let me make a new one?
        Hide
        Honghua Feng added a comment -

        Yes, both taskLoop and run handle InterruptedException already(though some just handle for its own sleep), letting getTaskList()/taskLoop() just throw out IE and letting run() handle IE does make the code cleaner. Good to me

        btw: I made a new patch for HBASE-10521, would you help review it again? thanks

        Show
        Honghua Feng added a comment - Yes, both taskLoop and run handle InterruptedException already(though some just handle for its own sleep), letting getTaskList()/taskLoop() just throw out IE and letting run() handle IE does make the code cleaner. Good to me btw: I made a new patch for HBASE-10521 , would you help review it again? thanks
        Hide
        Nicolas Liochon added a comment -

        Oops, actually no. getTaskList should just throw InterruptedException, this exception is managed in the upper layers already. See attached patch. wdyt?

        Show
        Nicolas Liochon added a comment - Oops, actually no. getTaskList should just throw InterruptedException, this exception is managed in the upper layers already. See attached patch. wdyt?
        Hide
        Nicolas Liochon added a comment -

        Yes, I'm going to commit with a small change
        + LOG.warn("Interrupted while sleeping during get task list ...", e1);
        => not need to log the exception, so I will log this
        + LOG.warn("Interrupted while sleeping during get task list.");

        Show
        Nicolas Liochon added a comment - Yes, I'm going to commit with a small change + LOG.warn("Interrupted while sleeping during get task list ...", e1); => not need to log the exception, so I will log this + LOG.warn("Interrupted while sleeping during get task list.");
        Hide
        Honghua Feng added a comment -

        ping Nicolas Liochon, seems you're OK with v1, and v2's change is only to move the log from where we restore the interrupt status to where we receive the exception per your suggestion.

        Show
        Honghua Feng added a comment - ping Nicolas Liochon , seems you're OK with v1, and v2's change is only to move the log from where we restore the interrupt status to where we receive the exception per your suggestion.
        Hide
        stack added a comment -

        More uglyness but makes sense (smile). +1 by me.

        Show
        stack added a comment - More uglyness but makes sense (smile). +1 by me.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12628921/HBASE-10524-trunk_v2.patch
        against trunk revision .
        ATTACHMENT ID: 12628921

        +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 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +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 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//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/12628921/HBASE-10524-trunk_v2.patch against trunk revision . ATTACHMENT ID: 12628921 +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 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +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 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8688//console This message is automatically generated.
        Hide
        Honghua Feng added a comment -

        new patch attached per Nicolas Liochon's suggestion to move log from when restoring to when receiving the interruption.

        Show
        Honghua Feng added a comment - new patch attached per Nicolas Liochon 's suggestion to move log from when restoring to when receiving the interruption.
        Hide
        Nicolas Liochon added a comment -

        Agreed as well for SplitLogWorker. I would propose not to log the exception however. The existing code was doing it, but it seems wrong. We should log when we receive the interruption, not when we restore it.
        (note: you should be able to 'submit' a patch, this will trigger a hadoop-qa run).

        Show
        Nicolas Liochon added a comment - Agreed as well for SplitLogWorker. I would propose not to log the exception however. The existing code was doing it, but it seems wrong. We should log when we receive the interruption, not when we restore it. (note: you should be able to 'submit' a patch, this will trigger a hadoop-qa run).
        Hide
        Nicolas Liochon added a comment -

        Yeah, for HRegionServer I think you're right.

        Show
        Nicolas Liochon added a comment - Yeah, for HRegionServer I think you're right.
        Hide
        Honghua Feng added a comment -

        Some notes for this jira, Nicolas Liochon :

        1. agree no need to interrupt since thread exits after the containing loop ends, so no change to MemStoreFlusher.java and Leases.java
        2. for SplitLogWorker, no change in method run() for the same reason of above; correct the calling of interrupt() within while loop in SplitLogWorker.getTaskList()
        3. for HRegionServer.createRegionServerStatusStub(), restore interrupt status after while loop if it's ever interrupted within the loop
        Show
        Honghua Feng added a comment - Some notes for this jira, Nicolas Liochon : agree no need to interrupt since thread exits after the containing loop ends, so no change to MemStoreFlusher.java and Leases.java for SplitLogWorker, no change in method run() for the same reason of above; correct the calling of interrupt() within while loop in SplitLogWorker.getTaskList() for HRegionServer.createRegionServerStatusStub(), restore interrupt status after while loop if it's ever interrupted within the loop

          People

          • Assignee:
            Honghua Feng
            Reporter:
            Honghua Feng
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development