HBASE-25547: Thread pools should release unused resources #2922
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Feb 4, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Outdated
Show resolved
Hide resolved
| return maxPoolSize; | ||
| } | ||
|
|
||
| public ExecutorConfig setMaxPoolSize(int maxPoolSize) { |
There was a problem hiding this comment.
What if the maxPoolSize is set to less than the corePoolSize? Would it be worth to check and automatically set to same as corePoolSize?
There was a problem hiding this comment.
ThreadPoolConstructor throws in that case.
There was a problem hiding this comment.
Thanks @wchevreuil for your comments, got a couple of questions before I push a new rev.
Should we also make the type of threads queue configurable?
Just to be sure, you mean ExecutorType enum? Ya thats a good idea.
My understanding is that with unbounded queues, maxPoolSize has no effect.
Not sure I follow. maxPoolSize is the max number of threads not the size of backing workQueue (javadoc). Mind rephrasing the question?
My understanding is that maxPoolSize works as a cap mostly when queueing wasn't possible, for example, because the queue was full. Revisited ThreadPoolExecutor docs:
"
...
- If fewer than corePoolSize threads are running, the Executor always prefers adding a new thread rather than queuing.
- If corePoolSize or more threads are running, the Executor always prefers queuing a request rather than adding a new thread.
- If a request cannot be queued, a new thread is created unless this would exceed maximumPoolSize, in which case, the task will be rejected.
...
There are three general strategies for queuing:
...
Unbounded queues. Using an unbounded queue (for example a LinkedBlockingQueue without a predefined capacity) will cause new tasks to wait in the queue when all corePoolSize threads are busy. Thus, no more than corePoolSize threads will ever be created. (And the value of the maximumPoolSize therefore doesn't have any effect.)
...
"
There was a problem hiding this comment.
Agree and we use unbounded queue:
// work queue to use - unbounded queue
final BlockingQueue<Runnable> q = new LinkedBlockingQueue<>();
Looks like our implementation never needed to worry about maxPoolSize so far, interesting!
bharathv
commented
Feb 5, 2021
There was a problem hiding this comment.
Thanks @wchevreuil for your comments, got a couple of questions before I push a new rev.
Should we also make the type of threads queue configurable?
Just to be sure, you mean ExecutorType enum? Ya thats a good idea.
My understanding is that with unbounded queues, maxPoolSize has no effect.
Not sure I follow. maxPoolSize is the max number of threads not the size of backing workQueue (javadoc). Mind rephrasing the question?
| return maxPoolSize; | ||
| } | ||
|
|
||
| public ExecutorConfig setMaxPoolSize(int maxPoolSize) { |
There was a problem hiding this comment.
ThreadPoolConstructor throws in that case.
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Outdated
Show resolved
Hide resolved
virajjasani
reviewed
Feb 8, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Outdated
Show resolved
Hide resolved
| return maxPoolSize; | ||
| } | ||
|
|
||
| public ExecutorConfig setMaxPoolSize(int maxPoolSize) { |
There was a problem hiding this comment.
Agree and we use unbounded queue:
// work queue to use - unbounded queue
final BlockingQueue<Runnable> q = new LinkedBlockingQueue<>();
Looks like our implementation never needed to worry about maxPoolSize so far, interesting!
saintstack
approved these changes
Feb 8, 2021
| keepAliveTimeInMillis, TimeUnit.MILLISECONDS, q); | ||
| config.getCorePoolSize(), config.getMaxPoolSize(), | ||
| config.getKeepAliveTimeMillis(), TimeUnit.MILLISECONDS, q); | ||
| this.threadPoolExecutor.allowCoreThreadTimeOut(config.allowCoreThreadTimeout()); |
bharathv
force-pushed
the
HBASE-25547
branch
from
dd54ffb
to
492ec19
Compare
bharathv
commented
Feb 10, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Show resolved
Hide resolved
bharathv
force-pushed
the
HBASE-25547
branch
3 times, most recently
from
aa551b4
to
a8b6fd7
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Enabled timeout of core threads for some low priority thread pools to reclaim unused resources.
bharathv
force-pushed
the
HBASE-25547
branch
from
a8b6fd7
to
881e4d9
Compare
bharathv
commented
Feb 16, 2021
There was a problem hiding this comment.
@wchevreuil @virajjasani Any more comments? Rebased on the latest tip fwiw.
|
🎊 +1 overall
This message was automatically generated. |
|
So far, we are starting off with terminating core worker threads if no tasks arrive in keepAlive time only for EventTypes: |
Instead moved it to the actual method. Copy-pasting it to multiple places seems redundant.
JVM thread creation is expensive. It involves initializing the stack memory, creating a corresponding native thread etc.. We may not want to do it for all thread pools for performance sake. We can add other thread pools over time as we see fit but turning it on by default might have some unintended and hard to track down performance issues. |
|
🎊 +1 overall
This message was automatically generated. |
|
Oh Great, you are right. Thread creation should be considered only if necessary for tasks like merge regions (recent change), where we are certain that it might happen for less than 1% of total cluster's available times. We are good with default value false. |
virajjasani
approved these changes
Feb 16, 2021
Comment on lines
+210
to
+214
| /** | ||
| * Allows timing out of core threads. Good to set this for non-critical thread pools for | ||
| * release of unused resources. Refer to {@link ThreadPoolExecutor#allowCoreThreadTimeOut} | ||
| * for additional details. | ||
| */ |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Feb 16, 2021
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@bharathv When you commit, can you please mention 4 EventTypes for which this commit is going to consider releasing unused core threads in thread pools? Maybe you can mention them in Release notes as well. |
|
I just figured that I missed the commit that rolls the op-code into the ExecutorConfig, let me create an addendum. |
ddupg
pushed a commit
to ddupg/hbase
that referenced
this pull request
Feb 19, 2021
Plumbs the configuration needed to enable core thread timeout on non-critical thread pools. Currently only enabled for thread pools with op-codes RS_LOG_REPLAY_OPS, RS_PARALLEL_SEEK, MASTER_SNAPSHOT_OPERATIONS, MASTER_MERGE_OPERATIONS. Others can be added later as needed. Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Feb 24, 2021
Plumbs the configuration needed to enable core thread timeout on non-critical thread pools. Currently only enabled for thread pools with op-codes RS_LOG_REPLAY_OPS, RS_PARALLEL_SEEK, MASTER_SNAPSHOT_OPERATIONS, MASTER_MERGE_OPERATIONS. Others can be added later as needed. Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
bharathv
added a commit
to bharathv/hbase
that referenced
this pull request
Mar 8, 2021
Plumbs the configuration needed to enable core thread timeout on non-critical thread pools. Currently only enabled for thread pools with op-codes RS_LOG_REPLAY_OPS, RS_PARALLEL_SEEK, MASTER_SNAPSHOT_OPERATIONS, MASTER_MERGE_OPERATIONS. Others can be added later as needed. Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit 618236d)
bharathv
added a commit
that referenced
this pull request
Mar 9, 2021
* HBASE-25547: Thread pools should release unused resources (#2922) Plumbs the configuration needed to enable core thread timeout on non-critical thread pools. Currently only enabled for thread pools with op-codes RS_LOG_REPLAY_OPS, RS_PARALLEL_SEEK, MASTER_SNAPSHOT_OPERATIONS, MASTER_MERGE_OPERATIONS. Others can be added later as needed. Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit 618236d) * HBASE-25547 (addendum): Roll ExecutorType into ExecutorConfig Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> (cherry picked from commit 4c822d7)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Still marked as a WIP because the defaults have not been tuned yet.
All the code is in place, I still need to identify the low priority
thread pools and tune them to release unused threads quickly.