hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: HBASE-2832: Priorities and multi-threading for MemStore flushing
Date Tue, 13 Jul 2010 04:55:02 GMT

-----------------------------------------------------------
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
> 
>


Mime
View raw message