HBase
  1. HBase
  2. HBASE-3043

'hbase-daemon.sh stop regionserver' should kill compactions that are in progress

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.89.20100621, 0.90.0
    • Fix Version/s: 0.90.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      During rolling restarts, we'll occasionally get into a situation with our 100-node cluster where a RS stop takes 5-10 minutes. The problem is that the RS is undergoing a compaction and won't stop until it is complete. In a stop situation, it would be preferable to preempt the compaction, delete the newly-created compaction file, and try again once the cluster is restarted.

      1. HBASE-3043_0.90.patch
        13 kB
        Nicolas Spiegelberg
      2. HBASE-3043_0.89.patch
        13 kB
        Nicolas Spiegelberg

        Issue Links

          Activity

          Nicolas Spiegelberg created issue -
          Hide
          Jean-Daniel Cryans added a comment -

          +1, although I don't think it's going to be included in 0924 unless the RC's sunk.

          Show
          Jean-Daniel Cryans added a comment - +1, although I don't think it's going to be included in 0924 unless the RC's sunk.
          Hide
          Nicolas Spiegelberg added a comment -

          I'm developing this patch against the 0.89 branch that we're running. It should work fine with 0.90, but I just wanted to note that in case any problems occur

          Show
          Nicolas Spiegelberg added a comment - I'm developing this patch against the 0.89 branch that we're running. It should work fine with 0.90, but I just wanted to note that in case any problems occur
          Nicolas Spiegelberg made changes -
          Field Original Value New Value
          Attachment HBASE-3043_0.90.patch [ 12456219 ]
          Attachment HBASE-3043_0.89.patch [ 12456220 ]
          Nicolas Spiegelberg made changes -
          Fix Version/s 0.90.0 [ 12313607 ]
          Affects Version/s 0.90.0 [ 12313607 ]
          Hide
          Nicolas Spiegelberg added a comment -

          NOTE: The default writeCheckInterval of 10MB came out to about 2sec between checks on my system. I choose a high number so minimize the performance impact on stores while maintaining a decently-responsive delay.

          Show
          Nicolas Spiegelberg added a comment - NOTE: The default writeCheckInterval of 10MB came out to about 2sec between checks on my system. I choose a high number so minimize the performance impact on stores while maintaining a decently-responsive delay.
          Nicolas Spiegelberg made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Pranav Khaitan added a comment -

          Patch looks good!

          One possibility is to maintain the variable 'WriteState writestate' as private and add a set method.

          Surprising that the following lock was never being unlocked before.
          void interruptIfNecessary() {
          if (lock.tryLock()) {

          • this.interrupt();
            + try { + this.interrupt(); + }

            finally

            { + lock.unlock(); + }

            }
            }

          Show
          Pranav Khaitan added a comment - Patch looks good! One possibility is to maintain the variable 'WriteState writestate' as private and add a set method. Surprising that the following lock was never being unlocked before. void interruptIfNecessary() { if (lock.tryLock()) { this.interrupt(); + try { + this.interrupt(); + } finally { + lock.unlock(); + } } }
          Hide
          stack added a comment -

          Looks grand to me Nicolas. Answer Pranav above and then I'll commit.

          Show
          stack added a comment - Looks grand to me Nicolas. Answer Pranav above and then I'll commit.
          Hide
          stack added a comment -

          Oh, one thought, if we are interrupted, we do not seem to cleanup after ourselves. Is the thought that cleanup happens when the region is next opened?

          Show
          stack added a comment - Oh, one thought, if we are interrupted, we do not seem to cleanup after ourselves. Is the thought that cleanup happens when the region is next opened?
          Hide
          Nicolas Spiegelberg added a comment -

          Pranav's comments:
          1) On WriteState variable privacy: 6 of one, half-dozen of the other. I made sure the WriteState variable was package private. I was looking at possibly some more unit tests dealing with our write state, so I didn't want to write a bunch of accessors just to deal with unit tests. In the unit test case, we don't really need to worry about synchronization either. My thought was to add accessor methods if we're going to use it outside of a unit test. Okay?
          2) The lack of unlock() actually could have caused some extremely-rare deadlock conditions but only on exit, so no one's probably run across it. Just mainly wanted to fix poor practice.

          Stack's comment:
          Your thought is correct. However, I do need to make a small change that I had done internally, but lost when I refactored. This works because of some subtle interactions between server.stopRequested(), CompactSplitThread.lock, & HRegion.writeState.writesEnabled. States that can happen:
          1) We get the lock & interrupt compactionQueue.poll(). It throws an InterruptedException, which calls continue, which fails the next while() check, which finishes the close
          2) We get the lock & interrupt, but the thread is somewhere between the poll() and the lock(). [In new patch] CompactSplitThread.run() queries stopRequested() immediately after getting the lock(), which skips the compact/split code to return to the while() check and ...
          3) We don't get the lock. HRegionServer.run() calls closeAllRegions(), which calls HRegion.close(), which sets the writeState. The compaction sees this, throws an InterruptedIOE, which is aborts the current compaction, goes to the while() check in CompactSplitThread.run() and ...

          Show
          Nicolas Spiegelberg added a comment - Pranav's comments: 1) On WriteState variable privacy: 6 of one, half-dozen of the other. I made sure the WriteState variable was package private. I was looking at possibly some more unit tests dealing with our write state, so I didn't want to write a bunch of accessors just to deal with unit tests. In the unit test case, we don't really need to worry about synchronization either. My thought was to add accessor methods if we're going to use it outside of a unit test. Okay? 2) The lack of unlock() actually could have caused some extremely-rare deadlock conditions but only on exit, so no one's probably run across it. Just mainly wanted to fix poor practice. Stack's comment: Your thought is correct. However, I do need to make a small change that I had done internally, but lost when I refactored. This works because of some subtle interactions between server.stopRequested(), CompactSplitThread.lock, & HRegion.writeState.writesEnabled. States that can happen: 1) We get the lock & interrupt compactionQueue.poll(). It throws an InterruptedException, which calls continue, which fails the next while() check, which finishes the close 2) We get the lock & interrupt, but the thread is somewhere between the poll() and the lock(). [In new patch] CompactSplitThread.run() queries stopRequested() immediately after getting the lock(), which skips the compact/split code to return to the while() check and ... 3) We don't get the lock. HRegionServer.run() calls closeAllRegions(), which calls HRegion.close(), which sets the writeState. The compaction sees this, throws an InterruptedIOE, which is aborts the current compaction, goes to the while() check in CompactSplitThread.run() and ...
          Hide
          Pranav Khaitan added a comment -

          Nicolas: great catch about that lock! That may have led to a deadlock
          The access issue can be ignored as it was just a trivial thing.

          Show
          Pranav Khaitan added a comment - Nicolas: great catch about that lock! That may have led to a deadlock The access issue can be ignored as it was just a trivial thing.
          Hide
          Nicolas Spiegelberg added a comment -

          Patches, updated after peer review. Also changed closeCheckInterval to static to fix TestHeapSize (no current reason for it to be per-instance)

          Show
          Nicolas Spiegelberg added a comment - Patches, updated after peer review. Also changed closeCheckInterval to static to fix TestHeapSize (no current reason for it to be per-instance)
          Nicolas Spiegelberg made changes -
          Attachment HBASE-3043_0.89.patch [ 12456258 ]
          Attachment HBASE-3043_0.90.patch [ 12456259 ]
          Nicolas Spiegelberg made changes -
          Attachment HBASE-3043_0.89.patch [ 12456220 ]
          Nicolas Spiegelberg made changes -
          Attachment HBASE-3043_0.90.patch [ 12456219 ]
          Hide
          stack added a comment -

          @Nicolas Excellent... Let me apply (running tests now to check nothing broke)

          Show
          stack added a comment - @Nicolas Excellent... Let me apply (running tests now to check nothing broke)
          Hide
          stack added a comment -

          Committed. Thanks for the sweet patch Nicolas.

          Show
          stack added a comment - Committed. Thanks for the sweet patch Nicolas.
          stack made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Fix Version/s 0.89.20100924 [ 12315366 ]
          Resolution Fixed [ 1 ]
          Nicolas Spiegelberg made changes -
          Link This issue is related to HBASE-2228 [ HBASE-2228 ]
          Hide
          Kannan Muthukkaruppan added a comment -

          I love how this patch also speeds up operations like table disables, region reassignments etc. which might otherwise be stuck on compactions to finish.

          Show
          Kannan Muthukkaruppan added a comment - I love how this patch also speeds up operations like table disables, region reassignments etc. which might otherwise be stuck on compactions to finish.
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          5d 1h 44m 1 Nicolas Spiegelberg 03/Oct/10 03:45
          Patch Available Patch Available Resolved Resolved
          1d 15h 9m 1 stack 04/Oct/10 18:54

            People

            • Assignee:
              Nicolas Spiegelberg
              Reporter:
              Nicolas Spiegelberg
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development