----------------------------------------------------------- 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 On 2010-07-12 21:41:44, Jonathan Gray wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/301/ > ----------------------------------------------------------- > > (Updated 2010-07-12 21:41:44) > > > 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 > >
