hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: Unit test and fix for multi-threaded executor service
Date Sat, 23 Oct 2010 05:42:34 GMT


> On 2010-10-22 19:12:22, Benoit Sigoure wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java, line
288
> > <http://review.cloudera.org/r/1064/diff/2/?file=15331#file15331line288>
> >
> >     Why is this class public?
> >     
> >     HBasePriorityBlockingQueue is a very bad class name.  BoundedPriorityBlockingQueue
would be better.
> >     
> >     It's annoying that PriorityQueue is unbounded, and because of that PriorityBlockingQueue
is unbounded too.

Its gone now in most recent patch posted to 3139.

Whats wrong w/ it being unbounded? Otherwise, all clients need to be able to deal with the
RejectedExecutionException.  What they going to do w/ the rejected event?  Hold it? Throw
it away?  If queue grows massive something is badly wrong.   If this becomes an issue we'll
refactor to deal with RejectedExecutionException and throw the exception all the ways out
to the remote service invoker.   Could be worse, could be unbounded thread pool. 


> On 2010-10-22 19:12:22, Benoit Sigoure wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java, line
305
> > <http://review.cloudera.org/r/1064/diff/2/?file=15331#file15331line305>
> >
> >     It's far cheaper to call super.size() [one lock acquisition] than getActiveCount()
[one lock acquisition + 1 volatile reads per worker thread]
> >     
> >     Plus, super.size() returns a consistent value, whereas getActiveCount() doesn't
guarantee an exact value (because each volatile read is done independently, without consistency
guarantee across all reads).
> >     
> >     You would also no longer need to retain a reference to the thread pool from
this class.

Good point (Class is now gone though as per above).


> On 2010-10-22 19:12:22, Benoit Sigoure wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java, line
306
> > <http://review.cloudera.org/r/1064/diff/2/?file=15331#file15331line306>
> >
> >     This sort of defeats the purpose of having a priority queue.  If the priority
queue is "full", you want to reject whichever element has the lowest priority, if the one
you're trying to enqueue has a higher priority.
> >     
> >     If you don't need a priority queue, you can switch to an ArrayBlockingQueue.

I switched it to LinkedBlockingQueue.  I want the unboundedness.


- stack


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1064/#review1629
-----------------------------------------------------------


On 2010-10-21 14:59:17, Jonathan Gray wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1064/
> -----------------------------------------------------------
> 
> (Updated 2010-10-21 14:59:17)
> 
> 
> Review request for hbase and stack.
> 
> 
> Summary
> -------
> 
> See HBASE-3139
> 
> 
> Diffs
> -----
> 
>   trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 1026145 
>   trunk/src/test/java/org/apache/hadoop/hbase/executor/TestExecutorService.java PRE-CREATION

> 
> Diff: http://review.cloudera.org/r/1064/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jonathan
> 
>


Mime
View raw message