hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "HBase Review Board (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HBASE-2832) Priorities and multi-threading for MemStore flushing
Date Tue, 13 Jul 2010 05:45:50 GMT

    [ https://issues.apache.org/jira/browse/HBASE-2832?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887661#action_12887661
] 

HBase Review Board commented on HBASE-2832:
-------------------------------------------

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


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > 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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line
20
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line20>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line
41
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line41>
bq.  >
bq.  >     Does the base class have to know of all subclasses?
bq.  >     
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line
52
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line52>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line
151
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line151>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java, line
198
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2551#file2551line198>
bq.  >
bq.  >     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?


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java,
line 78
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2552#file2552line78>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java,
line 157
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2552#file2552line157>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line
1032
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2555#file2555line1032>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line
270
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2556#file2556line270>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java, line
339
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2556#file2556line339>
bq.  >
bq.  >     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.


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java,
line 76
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line76>
bq.  >
bq.  >     Your nice formatting here will not come out in javadoc.

throwing <pre> tags around it enough?


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java,
line 127
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line127>
bq.  >
bq.  >     Can these be final?

done


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java,
line 148
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line148>
bq.  >
bq.  >     Yeah, why does eventType have to be passed?  Once in here, you know what to
pass?

done


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java,
line 178
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line178>
bq.  >
bq.  >     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?


bq.  On 2010-07-12 21:55:02, stack wrote:
bq.  > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/FlushHandler.java,
line 220
bq.  > <http://review.hbase.org/r/301/diff/2/?file=2557#file2557line220>
bq.  >
bq.  >     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
-----------------------------------------------------------





> Priorities and multi-threading for MemStore flushing
> ----------------------------------------------------
>
>                 Key: HBASE-2832
>                 URL: https://issues.apache.org/jira/browse/HBASE-2832
>             Project: HBase
>          Issue Type: New Feature
>          Components: regionserver
>            Reporter: Jonathan Gray
>            Assignee: Jonathan Gray
>            Priority: Critical
>             Fix For: 0.90.0
>
>
> 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.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message