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