You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Jonathan Gray <jg...@apache.org> on 2010/10/21 23:57:44 UTC
Review Request: Unit test and fix for multi-threaded executor service
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1064/
-----------------------------------------------------------
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
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by Jonathan Gray <jg...@apache.org>.
> On 2010-10-22 19:12:22, Benoit Sigoure wrote:
> >
Hey Benoit. Sorry should have thrown a comment up here. This isn't going to get committed, Stack has a different implementation over in HBASE-3319. We'll eventually need a PQ but for now it'll get switched to a LinkedBlockingQueue and there's no extra class.
Thanks for review though. I kinda figured that getActiveCount() was not ideal but thanks for digging in.
- Jonathan
-----------------------------------------------------------
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
>
>
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by st...@duboce.net.
> 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
>
>
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by Benoit Sigoure <ts...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by Ted Yu <yu...@gmail.com>.
review board gave me timeout.
The reason for this change is that ThreadPoolExecutor would try to enqueue
before maxPoolSize is reached.
Please refer to ThreadPoolExecutor.execute() method.
On Fri, Oct 22, 2010 at 6:02 AM, <st...@duboce.net> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1064/#review1605
> -----------------------------------------------------------
>
> Ship it!
>
>
> +1 Missing explaination of why you have to make this crazy hbase queue in
> first place.
>
>
> trunk/src/main/java/org/apache/hadoop/hbase/executor/m2m-integration builds
> Service.java
> <http://review.cloudera.org/r/1064/#comment5413>
>
> Doc why this oddness.
>
>
>
> trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
> <http://review.cloudera.org/r/1064/#comment5412>
>
> Say why we even need this crazy queue; i.e. because excecutorservice is
> weird.
>
>
> - stack
>
>
> 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
> >
> >
>
>
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1064/#review1605
-----------------------------------------------------------
Ship it!
+1 Missing explaination of why you have to make this crazy hbase queue in first place.
trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
<http://review.cloudera.org/r/1064/#comment5413>
Doc why this oddness.
trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
<http://review.cloudera.org/r/1064/#comment5412>
Say why we even need this crazy queue; i.e. because excecutorservice is weird.
- stack
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
>
>
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1064/#review1632
-----------------------------------------------------------
Ship it!
Looks good to me. Nice test :)
- Jonathan
On 2010-10-22 23:27:28, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1064/
> -----------------------------------------------------------
>
> (Updated 2010-10-22 23:27:28)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> See HBASE-3139
>
>
> This addresses bug hbase-3139.
> http://issues.apache.org/jira/browse/hbase-3139
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 1026564
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1026564
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1026564
> trunk/src/test/java/org/apache/hadoop/hbase/executor/TestExecutorService.java 1026564
>
> Diff: http://review.cloudera.org/r/1064/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1064/
-----------------------------------------------------------
(Updated 2010-10-22 23:27:28.712219)
Review request for hbase and stack.
Changes
-------
Remove priority queue -- not needed. Fix naming format for threads. Make the thread pool max and corepool size the same so we have a fixed pool size. Adjust thread pool sizes down. They were upped when we weren't getting throughput when we thought executorservice was doing parallelism but wasnt'.
Summary
-------
See HBASE-3139
This addresses bug hbase-3139.
http://issues.apache.org/jira/browse/hbase-3139
Diffs (updated)
-----
trunk/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 1026564
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1026564
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1026564
trunk/src/test/java/org/apache/hadoop/hbase/executor/TestExecutorService.java 1026564
Diff: http://review.cloudera.org/r/1064/diff
Testing
-------
Thanks,
Jonathan
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1064/
-----------------------------------------------------------
(Updated 2010-10-22 23:20:18.196100)
Review request for hbase and stack.
Summary
-------
See HBASE-3139
This addresses bug hbase-3139.
http://issues.apache.org/jira/browse/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
Re: Review Request: Unit test and fix for multi-threaded executor service
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1064/
-----------------------------------------------------------
(Updated 2010-10-21 14:59:17.175384)
Review request for hbase and stack.
Changes
-------
Fixes whitespace
Summary
-------
See HBASE-3139
Diffs (updated)
-----
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