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