----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1064/#review1629 -----------------------------------------------------------
trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java <http://review.cloudera.org/r/1064/#comment5485> 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. trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java <http://review.cloudera.org/r/1064/#comment5486> This statement is unnecessary. trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java <http://review.cloudera.org/r/1064/#comment5488> 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. trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java <http://review.cloudera.org/r/1064/#comment5487> 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. - Benoit 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 > >
