HBase
  1. HBase
  2. HBASE-2832

Priorities and multi-threading for MemStore flushing

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: regionserver
    • Labels:
      None

      Description

      Similar to HBASE-1476 and HBASE-2646 which are for compactions, but do this for flushes.

      Flushing when we hit the normal flush size is a low priority flush. Other types of flushes (heap pressure, blocking client requests, etc) are high priority.

      Should have a tunable number of concurrent flushes.

      Will use the HBaseExecutorService and HBaseEventHandler introduced from master/zk changes.

        Issue Links

          Activity

          Hide
          Jonathan Gray added a comment -

          Linking to issues that are for compactions

          Show
          Jonathan Gray added a comment - Linking to issues that are for compactions
          Hide
          Jonathan Gray added a comment -

          Class comment from new FlushHandler that does a rundown of flushing in hbase:

          
          

          /**

          • Flushes are currently triggered when:
            *
          • <ol>
          • <li>MemStore size of an HRegion is > hbase.hregion.memstore.flush.size
          • (checked after every memstore insertion)
          • </li>
          • <li>Sum of MemStores of an HRegionServer are > total.memstore.heap
          • (checked after every memstore insertion)
          • </li>
          • <li>Number of HLogs of an HRegionServer are > max.hlogs
          • (checked after every hlog roll?) TODO: Verify
          • </li>
          • <li>HRegion is being closed
          • (when receiving message from master)
          • </li>
          • <li>HRegionServer is being quiesced
          • (when receiving message from master)
          • </li>
          • <li>Client manually triggers flush of an HRegion
          • (when receiving message from master)
          • </li>
          • <li>MemStore size of an HRegion is > memstore.flush.size *
          • hbase.hregion.memstore.multiplier
          • (checked before every memstore insertion)
          • </li>
          • </ol>
            *
          • There are 3 different types of flushes that correspond to these 6 events:
            *
          • <ol>
          • <li>
          • Low Priority Flush
            *
          • This occurs in response to #1.
            *
          • This is the lowest priority flush and does not need any tricks. All other
          • flush types should be completed before any of this type are done. The one
          • optimization it has is that if it determines that a compaction would be
          • triggered after the flush finished, it should cancel the flush and instead
          • trigger a CompactAndFlush.
          • </li>
          • <li>
          • High Priority Flush
            *
          • This occurs in response to #2, #3, #6, and #7.
            *
          • High priority flushes occur in response to memory pressure, WAL pressure,
          • or because a user has asked for the flush. These flushes should occur
          • before any low priority flushes are processed. They are only special
          • because of their priority, otherwise the implementation of the flush is
          • identical to a low priority flush.
            *
          • This flush type explicitly does not contain the CompactAndFlush check
          • because it wants to flush as fast as possible.
          • </li>
          • <li>
          • High Priority Double Flush
            *
          • TODO: Region closing currently does flushing in-band rather than through
          • the flush queue. Should move those into using handlers once we have
          • blocking call. Therefore, double-flush priority is not currently
          • used.
            *
          • TODO: Do we want a separate priority here once we do use this for closes?
          • Or are they just high priority flushes? The first one is even a
          • low priority flush, second one high priority?
            *
          • This occurs in response to #4.
            *
          • When an HRegion is being closed (but not when a cluster is being quiesced)
          • we want to minimize the amount of time the region is unavailable. To do
          • this we do a double flush. A flush is done, then the region is closed,
          • then an additional flush is done before the region is available to be
          • re-opened. This should happen when the Master asks a region to close
          • because of reassignment.
            *
          • This may also occur before splits to reduce the amount of time the parent
          • is offline before the daughters come back online.
          • </li>
          • </ol>
            */
          Show
          Jonathan Gray added a comment - Class comment from new FlushHandler that does a rundown of flushing in hbase: /** Flushes are currently triggered when: * <ol> <li>MemStore size of an HRegion is > hbase.hregion.memstore.flush.size (checked after every memstore insertion) </li> <li>Sum of MemStores of an HRegionServer are > total.memstore.heap (checked after every memstore insertion) </li> <li>Number of HLogs of an HRegionServer are > max.hlogs (checked after every hlog roll?) TODO: Verify </li> <li>HRegion is being closed (when receiving message from master) </li> <li>HRegionServer is being quiesced (when receiving message from master) </li> <li>Client manually triggers flush of an HRegion (when receiving message from master) </li> <li>MemStore size of an HRegion is > memstore.flush.size * hbase.hregion.memstore.multiplier (checked before every memstore insertion) </li> </ol> * There are 3 different types of flushes that correspond to these 6 events: * <ol> <li> Low Priority Flush * This occurs in response to #1. * This is the lowest priority flush and does not need any tricks. All other flush types should be completed before any of this type are done. The one optimization it has is that if it determines that a compaction would be triggered after the flush finished, it should cancel the flush and instead trigger a CompactAndFlush. </li> <li> High Priority Flush * This occurs in response to #2, #3, #6, and #7. * High priority flushes occur in response to memory pressure, WAL pressure, or because a user has asked for the flush. These flushes should occur before any low priority flushes are processed. They are only special because of their priority, otherwise the implementation of the flush is identical to a low priority flush. * This flush type explicitly does not contain the CompactAndFlush check because it wants to flush as fast as possible. </li> <li> High Priority Double Flush * TODO: Region closing currently does flushing in-band rather than through the flush queue. Should move those into using handlers once we have blocking call. Therefore, double-flush priority is not currently used. * TODO: Do we want a separate priority here once we do use this for closes? Or are they just high priority flushes? The first one is even a low priority flush, second one high priority? * This occurs in response to #4. * When an HRegion is being closed (but not when a cluster is being quiesced) we want to minimize the amount of time the region is unavailable. To do this we do a double flush. A flush is done, then the region is closed, then an additional flush is done before the region is available to be re-opened. This should happen when the Master asks a region to close because of reassignment. * This may also occur before splits to reduce the amount of time the parent is offline before the daughters come back online. </li> </ol> */
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/301/
          -----------------------------------------------------------

          (Updated 2010-07-12 20:39:44.545617)

          Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

          Summary
          -------

          Adds support for priorities and concurrency to regionserver flushing.

          • Adds support for RS-side events/handlers/executors
          • Adds support for prioritized HBaseEventHandlers
          • Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler.
          • Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests.
          • Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now.
          • Something weird not passing in tests with multiple masters and regionservers, still working that out.

          This addresses bug HBASE-2832.
          http://issues.apache.org/jira/browse/HBASE-2832

          Diffs


          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION

          Diff: http://review.hbase.org/r/301/diff

          Testing
          -------

          Adds TestFlushHandler which passes. Working on getting unit tests passing now, something related to the ExecutorService.

          Thanks,

          Jonathan

          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/301/ ----------------------------------------------------------- (Updated 2010-07-12 20:39:44.545617) Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan. Summary ------- Adds support for priorities and concurrency to regionserver flushing. Adds support for RS-side events/handlers/executors Adds support for prioritized HBaseEventHandlers Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler. Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests. Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now. Something weird not passing in tests with multiple masters and regionservers, still working that out. This addresses bug HBASE-2832 . http://issues.apache.org/jira/browse/HBASE-2832 Diffs trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION Diff: http://review.hbase.org/r/301/diff Testing ------- Adds TestFlushHandler which passes. Working on getting unit tests passing now, something related to the ExecutorService. Thanks, Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/301/
          -----------------------------------------------------------

          (Updated 2010-07-12 21:41:44.943871)

          Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

          Changes
          -------

          Fixes some issues introduced by also having executor services running on the RS-side. Specifically, shutdown hooks to the HBaseExecutorService would shutdown and clear all executor pools, rather than just those of the shutting down server instance.

          Now the map of services is always prefixed with the serverName and during shutdown we only actually shutdown those prefixed with the serverName.

          More unit tests are now passing, still finish the suite.

          Summary
          -------

          Adds support for priorities and concurrency to regionserver flushing.

          • Adds support for RS-side events/handlers/executors
          • Adds support for prioritized HBaseEventHandlers
          • Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler.
          • Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests.
          • Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now.
          • Something weird not passing in tests with multiple masters and regionservers, still working that out.

          This addresses bug HBASE-2832.
          http://issues.apache.org/jira/browse/HBASE-2832

          Diffs (updated)


          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION

          Diff: http://review.hbase.org/r/301/diff

          Testing
          -------

          Adds TestFlushHandler which passes. Working on getting unit tests passing now, something related to the ExecutorService.

          Thanks,

          Jonathan

          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/301/ ----------------------------------------------------------- (Updated 2010-07-12 21:41:44.943871) Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan. Changes ------- Fixes some issues introduced by also having executor services running on the RS-side. Specifically, shutdown hooks to the HBaseExecutorService would shutdown and clear all executor pools, rather than just those of the shutting down server instance. Now the map of services is always prefixed with the serverName and during shutdown we only actually shutdown those prefixed with the serverName. More unit tests are now passing, still finish the suite. Summary ------- Adds support for priorities and concurrency to regionserver flushing. Adds support for RS-side events/handlers/executors Adds support for prioritized HBaseEventHandlers Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler. Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests. Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now. Something weird not passing in tests with multiple masters and regionservers, still working that out. This addresses bug HBASE-2832 . http://issues.apache.org/jira/browse/HBASE-2832 Diffs (updated) trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION Diff: http://review.hbase.org/r/301/diff Testing ------- Adds TestFlushHandler which passes. Working on getting unit tests passing now, something related to the ExecutorService. Thanks, Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: stack@duboce.net

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/301/#review377
          -----------------------------------------------------------

          I like what this adds but I'm not mad about the foundation its built upon. The EventType stuff seems off to me.

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
          <http://review.hbase.org/r/301/#comment1572>

          You couldn't call it EventHandler in the end?

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
          <http://review.hbase.org/r/301/#comment1573>

          Does the base class have to know of all subclasses?

          http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/beans/EventHandler.html and http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/util/EventObject.html are not of use here?

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
          <http://review.hbase.org/r/301/#comment1574>

          oh, ok... ignore above comment then.

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
          <http://review.hbase.org/r/301/#comment1575>

          A Comparable Runnable? Thats kinda odd? Runnable is a faceless Interface.. has nothing but a run in it... how could it be comparable? Should be Comparable<HBaseEventHandler>?

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
          <http://review.hbase.org/r/301/#comment1577>

          Why only this one handled in here? All others in subclass?

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
          <http://review.hbase.org/r/301/#comment1578>

          Can you not use enums here? RS_FLUSH_REGION.value rather than 64? (where value is datamember of the enum?)

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
          <http://review.hbase.org/r/301/#comment1579>

          Make it final then? Maybe you can't because its from Comparable.. but maybe you can.

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java
          <http://review.hbase.org/r/301/#comment1580>

          As above, this passed in int should be settting a data member (whats it setting otherwise, the enum index?)

          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java
          <http://review.hbase.org/r/301/#comment1581>

          Whats this wait for 60 seconds about? Hoping for an interrupt? Why hardcoded?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          <http://review.hbase.org/r/301/#comment1583>

          Why ain't this FlushEventType.startExecutorService? There are no master flushes so why this RS stuff in here?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java
          <http://review.hbase.org/r/301/#comment1584>

          Whats up? We just let the DroppedSnapshotException out now? Or do we not throw them anymore?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java
          <http://review.hbase.org/r/301/#comment1587>

          Why would a FlushHandler take anything but a FlushRegionEventType enum? Why even pass it in?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java
          <http://review.hbase.org/r/301/#comment1590>

          Your nice formatting here will not come out in javadoc.

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java
          <http://review.hbase.org/r/301/#comment1592>

          Excellent class doc. Needs to make its way out to the hbase 'book'.

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java
          <http://review.hbase.org/r/301/#comment1593>

          Can these be final?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java
          <http://review.hbase.org/r/301/#comment1595>

          Yeah, why does eventType have to be passed? Once in here, you know what to pass?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java
          <http://review.hbase.org/r/301/#comment1597>

          If 1, 2, 3, you don't need to specify? Just use enum ordinal?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java
          <http://review.hbase.org/r/301/#comment1599>

          Low and high priority do same thing?

          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java
          <http://review.hbase.org/r/301/#comment1600>

          OK, you moved it here... thats good.

          • stack
          Show
          HBase Review Board added a comment - Message from: stack@duboce.net ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/301/#review377 ----------------------------------------------------------- I like what this adds but I'm not mad about the foundation its built upon. The EventType stuff seems off to me. trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java < http://review.hbase.org/r/301/#comment1572 > You couldn't call it EventHandler in the end? trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java < http://review.hbase.org/r/301/#comment1573 > Does the base class have to know of all subclasses? http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/beans/EventHandler.html and http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/util/EventObject.html are not of use here? trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java < http://review.hbase.org/r/301/#comment1574 > oh, ok... ignore above comment then. trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java < http://review.hbase.org/r/301/#comment1575 > A Comparable Runnable? Thats kinda odd? Runnable is a faceless Interface.. has nothing but a run in it... how could it be comparable? Should be Comparable<HBaseEventHandler>? trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java < http://review.hbase.org/r/301/#comment1577 > Why only this one handled in here? All others in subclass? trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java < http://review.hbase.org/r/301/#comment1578 > Can you not use enums here? RS_FLUSH_REGION.value rather than 64? (where value is datamember of the enum?) trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java < http://review.hbase.org/r/301/#comment1579 > Make it final then? Maybe you can't because its from Comparable.. but maybe you can. trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java < http://review.hbase.org/r/301/#comment1580 > As above, this passed in int should be settting a data member (whats it setting otherwise, the enum index?) trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java < http://review.hbase.org/r/301/#comment1581 > Whats this wait for 60 seconds about? Hoping for an interrupt? Why hardcoded? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < http://review.hbase.org/r/301/#comment1583 > Why ain't this FlushEventType.startExecutorService? There are no master flushes so why this RS stuff in here? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java < http://review.hbase.org/r/301/#comment1584 > Whats up? We just let the DroppedSnapshotException out now? Or do we not throw them anymore? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java < http://review.hbase.org/r/301/#comment1587 > Why would a FlushHandler take anything but a FlushRegionEventType enum? Why even pass it in? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java < http://review.hbase.org/r/301/#comment1590 > Your nice formatting here will not come out in javadoc. trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java < http://review.hbase.org/r/301/#comment1592 > Excellent class doc. Needs to make its way out to the hbase 'book'. trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java < http://review.hbase.org/r/301/#comment1593 > Can these be final? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java < http://review.hbase.org/r/301/#comment1595 > Yeah, why does eventType have to be passed? Once in here, you know what to pass? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java < http://review.hbase.org/r/301/#comment1597 > If 1, 2, 3, you don't need to specify? Just use enum ordinal? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java < http://review.hbase.org/r/301/#comment1599 > Low and high priority do same thing? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java < http://review.hbase.org/r/301/#comment1600 > OK, you moved it here... thats good. stack
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@apache.org>

          On 2010-07-12 21:55:02, stack wrote:

          > I like what this adds but I'm not mad about the foundation its built upon. The EventType stuff seems off to me.

          Thanks for good review stack.

          Let's get this event stuff right because it's a huge part of the master changes.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 20

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line20>

          >

          > You couldn't call it EventHandler in the end?

          Yup, we can. Will be EventType/EventHandler with master changes. Requires the rest of the ZK cleanup to do the full changeover.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 41

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line41>

          >

          > Does the base class have to know of all subclasses?

          >

          > http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/beans/EventHandler.html and http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/util/EventObject.html are not of use here?

          Not sure what you're saying with java beans classes? You want to reuse those? These are quite specialized.

          We went over this stuff a few times when we did that big group review of that first master zk patch.

          Basically what we ended up doing was trying to keep the places things are tied together (handlers, executors, types) in enums and in as few places as possible. You add new executor service types in the ExecutorService class, but otherwise all the declaring/connecting of these things is done within HBaseEventHandler.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 52

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line52>

          >

          > A Comparable Runnable? Thats kinda odd? Runnable is a faceless Interface.. has nothing but a run in it... how could it be comparable? Should be Comparable<HBaseEventHandler>?

          I tried a few different approaches to have prioritized stuff.

          The underlying data structure expected by the java executor services are BlockingQueue<Runnable>. It's trivial then to make the actual queue a PriorityBlockingQueue<Runnable> which then just requires whatever you put in there to implement Comparable<Runnable>. In the compareTo we know that we will only be compared to other HBaseEventHandlers, so we can cast and do priority/FIFO comparisons.

          I'm pretty open to other approaches (I did Comparable<HBaseEventHandler> in one attempt already) but this turned out to be the cleanest. Now the handlers need only override a single getPriority() method rather than also be comparables themselves.

          Being a Comparable<HBEventHandler> means we'll have to not use a java executor service directly, or have two separate queues. It always starts to get a bit awkward which is why I ended up this way.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 151

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line151>

          >

          > Why only this one handled in here? All others in subclass?

          hmm, not so? this is only RS event right now. method immediately above it, getMasterExecutorForEvent(), is the same style and is already committed doing master open/close handlers.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 198

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line198>

          >

          > Can you not use enums here? RS_FLUSH_REGION.value rather than 64? (where value is datamember of the enum?)

          case statements must use constants. i could switch to else if?

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 78

          > <http://review.hbase.org/r/301/diff/2/?file=2552#file2552line78>

          >

          > As above, this passed in int should be settting a data member (whats it setting otherwise, the enum index?)

          i'm not totally clear on this int stuff with the enums. on the event types, we actually persist them, so I understand wanting the byte/int value. not sure here, we can just take it out? there is intValue/value in there.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 157

          > <http://review.hbase.org/r/301/diff/2/?file=2552#file2552line157>

          >

          > Whats this wait for 60 seconds about? Hoping for an interrupt? Why hardcoded?

          Need to figure out what to do here. Wait indefinitely?

          Basically, we shutdown the executor services gracefully at first (we let running and submitted tasks finish). Then we'll wait for a certain period of time before interrupting the threads running.

          I felt like it was weird to wait indefinitely but this was the behavior previously. I guess I should set to 0?

          (For example, MemStoreFlusher has a lock that prevents interruption while a flush is running, so it's equivalent to a graceful shutdown)

          I suppose there is a difference here which is this also includes items waiting in the queue.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1032

          > <http://review.hbase.org/r/301/diff/2/?file=2555#file2555line1032>

          >

          > Why ain't this FlushEventType.startExecutorService? There are no master flushes so why this RS stuff in here?

          There is separation between what runs on master side and what runs on RS side. Different mapings.

          This may not even be necessary anymore, there was some reason for the separation, need to ask Karthik on this.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line 270

          > <http://review.hbase.org/r/301/diff/2/?file=2556#file2556line270>

          >

          > Whats up? We just let the DroppedSnapshotException out now? Or do we not throw them anymore?

          As per description, handling of this moved to FlushHandler.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line 339

          > <http://review.hbase.org/r/301/diff/2/?file=2556#file2556line339>

          >

          > Why would a FlushHandler take anything but a FlushRegionEventType enum? Why even pass it in?

          It's not necessary for this one. Some of the other events take multiple event types (for example, we share a handler for opening/opened events).

          Will clean that up.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 76

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line76>

          >

          > Your nice formatting here will not come out in javadoc.

          throwing <pre> tags around it enough?

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 127

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line127>

          >

          > Can these be final?

          done

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 148

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line148>

          >

          > Yeah, why does eventType have to be passed? Once in here, you know what to pass?

          done

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 178

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line178>

          >

          > If 1, 2, 3, you don't need to specify? Just use enum ordinal?

          yeah it does. seemed clearer to have the ints here i think. i have gone back and forth several times and whether to have these ints in the enums or not, i'm not sure why i ended up with all of them having it.

          so you think it's better i completely remove the ints? what about in EventHandler where we persist it sometimes?

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 220

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line220>

          >

          > Low and high priority do same thing?

          yup. i had/have bigger plans to get rid of MemStoreFlusher so that FlushHandler actually does the checks and stuff. For now it's still in the MemStoreFlusher thread and just the flushing is multi-threaded in here.

          until that happens, same thing, that's why they call the same method, just a different log message.

          so they do the same flush but they carry a different priority so will be executed in priority order.

          • Jonathan

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/301/#review377
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> On 2010-07-12 21:55:02, stack wrote: > I like what this adds but I'm not mad about the foundation its built upon. The EventType stuff seems off to me. Thanks for good review stack. Let's get this event stuff right because it's a huge part of the master changes. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 20 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line20 > > > You couldn't call it EventHandler in the end? Yup, we can. Will be EventType/EventHandler with master changes. Requires the rest of the ZK cleanup to do the full changeover. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 41 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line41 > > > Does the base class have to know of all subclasses? > > http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/beans/EventHandler.html and http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/util/EventObject.html are not of use here? Not sure what you're saying with java beans classes? You want to reuse those? These are quite specialized. We went over this stuff a few times when we did that big group review of that first master zk patch. Basically what we ended up doing was trying to keep the places things are tied together (handlers, executors, types) in enums and in as few places as possible. You add new executor service types in the ExecutorService class, but otherwise all the declaring/connecting of these things is done within HBaseEventHandler. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 52 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line52 > > > A Comparable Runnable? Thats kinda odd? Runnable is a faceless Interface.. has nothing but a run in it... how could it be comparable? Should be Comparable<HBaseEventHandler>? I tried a few different approaches to have prioritized stuff. The underlying data structure expected by the java executor services are BlockingQueue<Runnable>. It's trivial then to make the actual queue a PriorityBlockingQueue<Runnable> which then just requires whatever you put in there to implement Comparable<Runnable>. In the compareTo we know that we will only be compared to other HBaseEventHandlers, so we can cast and do priority/FIFO comparisons. I'm pretty open to other approaches (I did Comparable<HBaseEventHandler> in one attempt already) but this turned out to be the cleanest. Now the handlers need only override a single getPriority() method rather than also be comparables themselves. Being a Comparable<HBEventHandler> means we'll have to not use a java executor service directly, or have two separate queues. It always starts to get a bit awkward which is why I ended up this way. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 151 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line151 > > > Why only this one handled in here? All others in subclass? hmm, not so? this is only RS event right now. method immediately above it, getMasterExecutorForEvent(), is the same style and is already committed doing master open/close handlers. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 198 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line198 > > > Can you not use enums here? RS_FLUSH_REGION.value rather than 64? (where value is datamember of the enum?) case statements must use constants. i could switch to else if? On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 78 > < http://review.hbase.org/r/301/diff/2/?file=2552#file2552line78 > > > As above, this passed in int should be settting a data member (whats it setting otherwise, the enum index?) i'm not totally clear on this int stuff with the enums. on the event types, we actually persist them, so I understand wanting the byte/int value. not sure here, we can just take it out? there is intValue/value in there. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 157 > < http://review.hbase.org/r/301/diff/2/?file=2552#file2552line157 > > > Whats this wait for 60 seconds about? Hoping for an interrupt? Why hardcoded? Need to figure out what to do here. Wait indefinitely? Basically, we shutdown the executor services gracefully at first (we let running and submitted tasks finish). Then we'll wait for a certain period of time before interrupting the threads running. I felt like it was weird to wait indefinitely but this was the behavior previously. I guess I should set to 0? (For example, MemStoreFlusher has a lock that prevents interruption while a flush is running, so it's equivalent to a graceful shutdown) I suppose there is a difference here which is this also includes items waiting in the queue. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1032 > < http://review.hbase.org/r/301/diff/2/?file=2555#file2555line1032 > > > Why ain't this FlushEventType.startExecutorService? There are no master flushes so why this RS stuff in here? There is separation between what runs on master side and what runs on RS side. Different mapings. This may not even be necessary anymore, there was some reason for the separation, need to ask Karthik on this. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line 270 > < http://review.hbase.org/r/301/diff/2/?file=2556#file2556line270 > > > Whats up? We just let the DroppedSnapshotException out now? Or do we not throw them anymore? As per description, handling of this moved to FlushHandler. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line 339 > < http://review.hbase.org/r/301/diff/2/?file=2556#file2556line339 > > > Why would a FlushHandler take anything but a FlushRegionEventType enum? Why even pass it in? It's not necessary for this one. Some of the other events take multiple event types (for example, we share a handler for opening/opened events). Will clean that up. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 76 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line76 > > > Your nice formatting here will not come out in javadoc. throwing <pre> tags around it enough? On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 127 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line127 > > > Can these be final? done On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 148 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line148 > > > Yeah, why does eventType have to be passed? Once in here, you know what to pass? done On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 178 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line178 > > > If 1, 2, 3, you don't need to specify? Just use enum ordinal? yeah it does. seemed clearer to have the ints here i think. i have gone back and forth several times and whether to have these ints in the enums or not, i'm not sure why i ended up with all of them having it. so you think it's better i completely remove the ints? what about in EventHandler where we persist it sometimes? On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 220 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line220 > > > Low and high priority do same thing? yup. i had/have bigger plans to get rid of MemStoreFlusher so that FlushHandler actually does the checks and stuff. For now it's still in the MemStoreFlusher thread and just the flushing is multi-threaded in here. until that happens, same thing, that's why they call the same method, just a different log message. so they do the same flush but they carry a different priority so will be executed in priority order. Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/301/#review377 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/301/
          -----------------------------------------------------------

          (Updated 2010-07-12 22:43:42.753269)

          Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

          Changes
          -------

          Incorporates changes I acknowledged from stack review.

          Summary
          -------

          Adds support for priorities and concurrency to regionserver flushing.

          • Adds support for RS-side events/handlers/executors
          • Adds support for prioritized HBaseEventHandlers
          • Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler.
          • Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests.
          • Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now.
          • Something weird not passing in tests with multiple masters and regionservers, still working that out.

          This addresses bug HBASE-2832.
          http://issues.apache.org/jira/browse/HBASE-2832

          Diffs (updated)


          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION

          Diff: http://review.hbase.org/r/301/diff

          Testing
          -------

          Adds TestFlushHandler which passes. Working on getting unit tests passing now, something related to the ExecutorService.

          Thanks,

          Jonathan

          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/301/ ----------------------------------------------------------- (Updated 2010-07-12 22:43:42.753269) Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan. Changes ------- Incorporates changes I acknowledged from stack review. Summary ------- Adds support for priorities and concurrency to regionserver flushing. Adds support for RS-side events/handlers/executors Adds support for prioritized HBaseEventHandlers Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler. Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests. Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now. Something weird not passing in tests with multiple masters and regionservers, still working that out. This addresses bug HBASE-2832 . http://issues.apache.org/jira/browse/HBASE-2832 Diffs (updated) trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION Diff: http://review.hbase.org/r/301/diff Testing ------- Adds TestFlushHandler which passes. Working on getting unit tests passing now, something related to the ExecutorService. Thanks, Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/301/
          -----------------------------------------------------------

          (Updated 2010-07-12 22:44:39.027666)

          Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

          Changes
          -------

          Unit tests are now passing.

          Summary
          -------

          Adds support for priorities and concurrency to regionserver flushing.

          • Adds support for RS-side events/handlers/executors
          • Adds support for prioritized HBaseEventHandlers
          • Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler.
          • Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests.
          • Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now.
          • Something weird not passing in tests with multiple masters and regionservers, still working that out.

          This addresses bug HBASE-2832.
          http://issues.apache.org/jira/browse/HBASE-2832

          Diffs


          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507
          trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION
          trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION

          Diff: http://review.hbase.org/r/301/diff

          Testing (updated)
          -------

          Adds TestFlushHandler which passes. Unit tests now passing.

          Thanks,

          Jonathan

          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/301/ ----------------------------------------------------------- (Updated 2010-07-12 22:44:39.027666) Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan. Changes ------- Unit tests are now passing. Summary ------- Adds support for priorities and concurrency to regionserver flushing. Adds support for RS-side events/handlers/executors Adds support for prioritized HBaseEventHandlers Flushing now happens through FlushHandler, a new HBaseEventHandler. There is an RS_FLUSHER executor pool that defaults to two threads right now but is also checking a conf value. There is a good bit of documentation in FlushHandler. Adds unit test TestFlushHandler. There is a nicer way to detect when flushes finish now for other tests. Handling of FS errors is pushed into FlushHandler now. The changes happening with the master rewrite introduce a ServerStatus interface (probably a RegionStatus for rs side) that will contain the necessary methods rather than using HRegionServer directly as is required for now. Something weird not passing in tests with multiple masters and regionservers, still working that out. This addresses bug HBASE-2832 . http://issues.apache.org/jira/browse/HBASE-2832 Diffs trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java 963507 trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java PRE-CREATION trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestFlushHandler.java PRE-CREATION Diff: http://review.hbase.org/r/301/diff Testing (updated) ------- Adds TestFlushHandler which passes. Unit tests now passing. Thanks, Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: stack@duboce.net

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 20

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line20>

          >

          > You couldn't call it EventHandler in the end?

          Jonathan Gray wrote:

          Yup, we can. Will be EventType/EventHandler with master changes. Requires the rest of the ZK cleanup to do the full changeover.

          ok

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 41

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line41>

          >

          > Does the base class have to know of all subclasses?

          >

          > http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/beans/EventHandler.html and http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/util/EventObject.html are not of use here?

          Jonathan Gray wrote:

          Not sure what you're saying with java beans classes? You want to reuse those? These are quite specialized.

          We went over this stuff a few times when we did that big group review of that first master zk patch.

          Basically what we ended up doing was trying to keep the places things are tied together (handlers, executors, types) in enums and in as few places as possible. You add new executor service types in the ExecutorService class, but otherwise all the declaring/connecting of these things is done within HBaseEventHandler.

          Regards the bean classes, they cannot be used here or there is nothing in them that can help here?

          Yeah, we went over this stuff in reviews and there were the same problems then. I thought we'd agreed to address them. My review is just surfacing again what was issue then.

          I thought we'd agreed to stuff like a handler per event type.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 52

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line52>

          >

          > A Comparable Runnable? Thats kinda odd? Runnable is a faceless Interface.. has nothing but a run in it... how could it be comparable? Should be Comparable<HBaseEventHandler>?

          Jonathan Gray wrote:

          I tried a few different approaches to have prioritized stuff.

          The underlying data structure expected by the java executor services are BlockingQueue<Runnable>. It's trivial then to make the actual queue a PriorityBlockingQueue<Runnable> which then just requires whatever you put in there to implement Comparable<Runnable>. In the compareTo we know that we will only be compared to other HBaseEventHandlers, so we can cast and do priority/FIFO comparisons.

          I'm pretty open to other approaches (I did Comparable<HBaseEventHandler> in one attempt already) but this turned out to be the cleanest. Now the handlers need only override a single getPriority() method rather than also be comparables themselves.

          Being a Comparable<HBEventHandler> means we'll have to not use a java executor service directly, or have two separate queues. It always starts to get a bit awkward which is why I ended up this way.

          Comparable<Runnable> punts completely on type checking. It says less than nothing about whats going on. We can't have an Interface that implements Comparable and Runnable and pass that?

          My sense is that the design is broke if we're doing weird stuff like this (I've not dug in as you have – just passing my sense).

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 151

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line151>

          >

          > Why only this one handled in here? All others in subclass?

          Jonathan Gray wrote:

          hmm, not so? this is only RS event right now. method immediately above it, getMasterExecutorForEvent(), is the same style and is already committed doing master open/close handlers.

          ok

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 198

          > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line198>

          >

          > Can you not use enums here? RS_FLUSH_REGION.value rather than 64? (where value is datamember of the enum?)

          Jonathan Gray wrote:

          case statements must use constants. i could switch to else if?

          Can't you use enums in switch statements?

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 78

          > <http://review.hbase.org/r/301/diff/2/?file=2552#file2552line78>

          >

          > As above, this passed in int should be settting a data member (whats it setting otherwise, the enum index?)

          Jonathan Gray wrote:

          i'm not totally clear on this int stuff with the enums. on the event types, we actually persist them, so I understand wanting the byte/int value. not sure here, we can just take it out? there is intValue/value in there.

          It seems odd. I can understand not wanting to use enum ordinals in case you want to insert events later but should be better way of doing this...

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 157

          > <http://review.hbase.org/r/301/diff/2/?file=2552#file2552line157>

          >

          > Whats this wait for 60 seconds about? Hoping for an interrupt? Why hardcoded?

          Jonathan Gray wrote:

          Need to figure out what to do here. Wait indefinitely?

          Basically, we shutdown the executor services gracefully at first (we let running and submitted tasks finish). Then we'll wait for a certain period of time before interrupting the threads running.

          I felt like it was weird to wait indefinitely but this was the behavior previously. I guess I should set to 0?

          (For example, MemStoreFlusher has a lock that prevents interruption while a flush is running, so it's equivalent to a graceful shutdown)

          I suppose there is a difference here which is this also includes items waiting in the queue.

          I ain't sure whats going on here exactly. Just commenting that the 60 seconds is odd.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1032

          > <http://review.hbase.org/r/301/diff/2/?file=2555#file2555line1032>

          >

          > Why ain't this FlushEventType.startExecutorService? There are no master flushes so why this RS stuff in here?

          Jonathan Gray wrote:

          There is separation between what runs on master side and what runs on RS side. Different mapings.

          This may not even be necessary anymore, there was some reason for the separation, need to ask Karthik on this.

          ok

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line 270

          > <http://review.hbase.org/r/301/diff/2/?file=2556#file2556line270>

          >

          > Whats up? We just let the DroppedSnapshotException out now? Or do we not throw them anymore?

          Jonathan Gray wrote:

          As per description, handling of this moved to FlushHandler.

          ok

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 76

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line76>

          >

          > Your nice formatting here will not come out in javadoc.

          Jonathan Gray wrote:

          throwing <pre> tags around it enough?

          yes

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 178

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line178>

          >

          > If 1, 2, 3, you don't need to specify? Just use enum ordinal?

          Jonathan Gray wrote:

          yeah it does. seemed clearer to have the ints here i think. i have gone back and forth several times and whether to have these ints in the enums or not, i'm not sure why i ended up with all of them having it.

          so you think it's better i completely remove the ints? what about in EventHandler where we persist it sometimes?

          You can persist the ordinal... Thats fine. There is even method in enum to give you enum if you only have ordinal.

          On 2010-07-12 21:55:02, stack wrote:

          > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 220

          > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line220>

          >

          > Low and high priority do same thing?

          Jonathan Gray wrote:

          yup. i had/have bigger plans to get rid of MemStoreFlusher so that FlushHandler actually does the checks and stuff. For now it's still in the MemStoreFlusher thread and just the flushing is multi-threaded in here.

          until that happens, same thing, that's why they call the same method, just a different log message.

          so they do the same flush but they carry a different priority so will be executed in priority order.

          k. maybe more comment so others ain't baffled as I?

          • stack

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/301/#review377
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: stack@duboce.net On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 20 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line20 > > > You couldn't call it EventHandler in the end? Jonathan Gray wrote: Yup, we can. Will be EventType/EventHandler with master changes. Requires the rest of the ZK cleanup to do the full changeover. ok On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 41 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line41 > > > Does the base class have to know of all subclasses? > > http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/beans/EventHandler.html and http://download.oracle.com/docs/cd/E17409_01/javase/6/docs/api/java/util/EventObject.html are not of use here? Jonathan Gray wrote: Not sure what you're saying with java beans classes? You want to reuse those? These are quite specialized. We went over this stuff a few times when we did that big group review of that first master zk patch. Basically what we ended up doing was trying to keep the places things are tied together (handlers, executors, types) in enums and in as few places as possible. You add new executor service types in the ExecutorService class, but otherwise all the declaring/connecting of these things is done within HBaseEventHandler. Regards the bean classes, they cannot be used here or there is nothing in them that can help here? Yeah, we went over this stuff in reviews and there were the same problems then. I thought we'd agreed to address them. My review is just surfacing again what was issue then. I thought we'd agreed to stuff like a handler per event type. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 52 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line52 > > > A Comparable Runnable? Thats kinda odd? Runnable is a faceless Interface.. has nothing but a run in it... how could it be comparable? Should be Comparable<HBaseEventHandler>? Jonathan Gray wrote: I tried a few different approaches to have prioritized stuff. The underlying data structure expected by the java executor services are BlockingQueue<Runnable>. It's trivial then to make the actual queue a PriorityBlockingQueue<Runnable> which then just requires whatever you put in there to implement Comparable<Runnable>. In the compareTo we know that we will only be compared to other HBaseEventHandlers, so we can cast and do priority/FIFO comparisons. I'm pretty open to other approaches (I did Comparable<HBaseEventHandler> in one attempt already) but this turned out to be the cleanest. Now the handlers need only override a single getPriority() method rather than also be comparables themselves. Being a Comparable<HBEventHandler> means we'll have to not use a java executor service directly, or have two separate queues. It always starts to get a bit awkward which is why I ended up this way. Comparable<Runnable> punts completely on type checking. It says less than nothing about whats going on. We can't have an Interface that implements Comparable and Runnable and pass that? My sense is that the design is broke if we're doing weird stuff like this (I've not dug in as you have – just passing my sense). On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 151 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line151 > > > Why only this one handled in here? All others in subclass? Jonathan Gray wrote: hmm, not so? this is only RS event right now. method immediately above it, getMasterExecutorForEvent(), is the same style and is already committed doing master open/close handlers. ok On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line 198 > < http://review.hbase.org/r/301/diff/2/?file=2551#file2551line198 > > > Can you not use enums here? RS_FLUSH_REGION.value rather than 64? (where value is datamember of the enum?) Jonathan Gray wrote: case statements must use constants. i could switch to else if? Can't you use enums in switch statements? On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 78 > < http://review.hbase.org/r/301/diff/2/?file=2552#file2552line78 > > > As above, this passed in int should be settting a data member (whats it setting otherwise, the enum index?) Jonathan Gray wrote: i'm not totally clear on this int stuff with the enums. on the event types, we actually persist them, so I understand wanting the byte/int value. not sure here, we can just take it out? there is intValue/value in there. It seems odd. I can understand not wanting to use enum ordinals in case you want to insert events later but should be better way of doing this... On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java, line 157 > < http://review.hbase.org/r/301/diff/2/?file=2552#file2552line157 > > > Whats this wait for 60 seconds about? Hoping for an interrupt? Why hardcoded? Jonathan Gray wrote: Need to figure out what to do here. Wait indefinitely? Basically, we shutdown the executor services gracefully at first (we let running and submitted tasks finish). Then we'll wait for a certain period of time before interrupting the threads running. I felt like it was weird to wait indefinitely but this was the behavior previously. I guess I should set to 0? (For example, MemStoreFlusher has a lock that prevents interruption while a flush is running, so it's equivalent to a graceful shutdown) I suppose there is a difference here which is this also includes items waiting in the queue. I ain't sure whats going on here exactly. Just commenting that the 60 seconds is odd. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1032 > < http://review.hbase.org/r/301/diff/2/?file=2555#file2555line1032 > > > Why ain't this FlushEventType.startExecutorService? There are no master flushes so why this RS stuff in here? Jonathan Gray wrote: There is separation between what runs on master side and what runs on RS side. Different mapings. This may not even be necessary anymore, there was some reason for the separation, need to ask Karthik on this. ok On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line 270 > < http://review.hbase.org/r/301/diff/2/?file=2556#file2556line270 > > > Whats up? We just let the DroppedSnapshotException out now? Or do we not throw them anymore? Jonathan Gray wrote: As per description, handling of this moved to FlushHandler. ok On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 76 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line76 > > > Your nice formatting here will not come out in javadoc. Jonathan Gray wrote: throwing <pre> tags around it enough? yes On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 178 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line178 > > > If 1, 2, 3, you don't need to specify? Just use enum ordinal? Jonathan Gray wrote: yeah it does. seemed clearer to have the ints here i think. i have gone back and forth several times and whether to have these ints in the enums or not, i'm not sure why i ended up with all of them having it. so you think it's better i completely remove the ints? what about in EventHandler where we persist it sometimes? You can persist the ordinal... Thats fine. There is even method in enum to give you enum if you only have ordinal. On 2010-07-12 21:55:02, stack wrote: > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java, line 220 > < http://review.hbase.org/r/301/diff/2/?file=2557#file2557line220 > > > Low and high priority do same thing? Jonathan Gray wrote: yup. i had/have bigger plans to get rid of MemStoreFlusher so that FlushHandler actually does the checks and stuff. For now it's still in the MemStoreFlusher thread and just the flushing is multi-threaded in here. until that happens, same thing, that's why they call the same method, just a different log message. so they do the same flush but they carry a different priority so will be executed in priority order. k. maybe more comment so others ain't baffled as I? stack ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/301/#review377 -----------------------------------------------------------
          Hide
          Jean-Daniel Cryans added a comment -

          Is this currently integrated in the master branch? If not, then either we finish and close it or punt to 0.92.0 as we have enough patches in flight.

          Show
          Jean-Daniel Cryans added a comment - Is this currently integrated in the master branch? If not, then either we finish and close it or punt to 0.92.0 as we have enough patches in flight.
          Hide
          Jonathan Gray added a comment -

          Priorities and multi-threading is added through EventHandlers/ExecutorService in the master branch.

          This jira and HBASE-2375 (and HBASE-2399) will hopefully be done for 0.90 but cannot be done until after the master is merged in. This should not be a blocker but I hope to do it for 0.90. Will punt in a few weeks if it doesn't look likely.

          Show
          Jonathan Gray added a comment - Priorities and multi-threading is added through EventHandlers/ExecutorService in the master branch. This jira and HBASE-2375 (and HBASE-2399 ) will hopefully be done for 0.90 but cannot be done until after the master is merged in. This should not be a blocker but I hope to do it for 0.90. Will punt in a few weeks if it doesn't look likely.
          Hide
          Jonathan Gray added a comment -

          Punting to 0.92 for now. The bigger compaction/flush improvements should happen in that version.

          Show
          Jonathan Gray added a comment - Punting to 0.92 for now. The bigger compaction/flush improvements should happen in that version.
          Hide
          Nicolas Spiegelberg added a comment -

          Not sure about the state of this JIRA (Review Board seems to be down). I believe the current design is to have thread-pools for compactions and to have one thread dedicated to kick in when we hit the block storefiles limit. One idea thrown around internally is having a dedicated compaction thread with a lowered 'hbase.hstore.compaction.max.size' that will kick in earlier and only compact small files. The idea is to keep the storefile count low for reads in the face of a couple long compactions (e.g. major compaction storm).

          Show
          Nicolas Spiegelberg added a comment - Not sure about the state of this JIRA (Review Board seems to be down). I believe the current design is to have thread-pools for compactions and to have one thread dedicated to kick in when we hit the block storefiles limit. One idea thrown around internally is having a dedicated compaction thread with a lowered 'hbase.hstore.compaction.max.size' that will kick in earlier and only compact small files. The idea is to keep the storefile count low for reads in the face of a couple long compactions (e.g. major compaction storm).
          Hide
          Todd Lipcon added a comment -

          fyi you can access the review using review.cloudera.org instead of review.hbase.org as the hostname. I just asked Stack to update the DNS.

          Show
          Todd Lipcon added a comment - fyi you can access the review using review.cloudera.org instead of review.hbase.org as the hostname. I just asked Stack to update the DNS.
          Hide
          Jonathan Gray added a comment -

          punting from 0.92. still needs to be done but should not be tied to a version until work is being actively done

          Show
          Jonathan Gray added a comment - punting from 0.92. still needs to be done but should not be tied to a version until work is being actively done
          Hide
          Jean-Daniel Cryans added a comment -

          Solved in HBASE-6466.

          Show
          Jean-Daniel Cryans added a comment - Solved in HBASE-6466 .

            People

            • Assignee:
              Jonathan Gray
              Reporter:
              Jonathan Gray
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development