wchevreuil commented on a change in pull request #2922:
URL: https://github.com/apache/hbase/pull/2922#discussion_r570169987
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
##########
@@ -182,12 +182,75 @@ public void delayedSubmit(EventHandler eh, long delay,
TimeUnit unit) {
return ret;
}
+ /**
+ * Configuration wrapper for {@link Executor}.
+ */
+ public static class ExecutorConfig {
+ // Refer to ThreadPoolExecutor javadoc for details of these configuration.
+ public static final long KEEP_ALIVE_TIME_MILLIS_DEFAULT = 1000;
+ private int corePoolSize = -1;
+ private int maxPoolSize = -1;
+ private boolean allowCoreThreadTimeout = false;
+ private long keepAliveTimeMillis = KEEP_ALIVE_TIME_MILLIS_DEFAULT;
+ private String name;
+
+ public int getMaxPoolSize() {
+ Preconditions.checkState(maxPoolSize >= 1, "Invalid max pool size: " +
maxPoolSize);
Review comment:
Should we check this upfront, in the related setter (or maybe at a non
default constructor)?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
##########
@@ -182,12 +182,75 @@ public void delayedSubmit(EventHandler eh, long delay,
TimeUnit unit) {
return ret;
}
+ /**
+ * Configuration wrapper for {@link Executor}.
+ */
+ public static class ExecutorConfig {
+ // Refer to ThreadPoolExecutor javadoc for details of these configuration.
+ public static final long KEEP_ALIVE_TIME_MILLIS_DEFAULT = 1000;
+ private int corePoolSize = -1;
+ private int maxPoolSize = -1;
+ private boolean allowCoreThreadTimeout = false;
+ private long keepAliveTimeMillis = KEEP_ALIVE_TIME_MILLIS_DEFAULT;
+ private String name;
+
+ public int getMaxPoolSize() {
+ Preconditions.checkState(maxPoolSize >= 1, "Invalid max pool size: " +
maxPoolSize);
+ return maxPoolSize;
+ }
+
+ public ExecutorConfig setMaxPoolSize(int maxPoolSize) {
Review 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?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
##########
@@ -182,12 +182,75 @@ public void delayedSubmit(EventHandler eh, long delay,
TimeUnit unit) {
return ret;
}
+ /**
+ * Configuration wrapper for {@link Executor}.
+ */
+ public static class ExecutorConfig {
+ // Refer to ThreadPoolExecutor javadoc for details of these configuration.
+ public static final long KEEP_ALIVE_TIME_MILLIS_DEFAULT = 1000;
+ private int corePoolSize = -1;
+ private int maxPoolSize = -1;
Review comment:
Could we define minimal valid defaults for this, since we are enforcing
checks later? Or maybe require setting these at construction time.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
##########
@@ -182,12 +182,75 @@ public void delayedSubmit(EventHandler eh, long delay,
TimeUnit unit) {
return ret;
}
+ /**
+ * Configuration wrapper for {@link Executor}.
+ */
+ public static class ExecutorConfig {
+ // Refer to ThreadPoolExecutor javadoc for details of these configuration.
+ public static final long KEEP_ALIVE_TIME_MILLIS_DEFAULT = 1000;
+ private int corePoolSize = -1;
+ private int maxPoolSize = -1;
+ private boolean allowCoreThreadTimeout = false;
+ private long keepAliveTimeMillis = KEEP_ALIVE_TIME_MILLIS_DEFAULT;
+ private String name;
+
+ public int getMaxPoolSize() {
+ Preconditions.checkState(maxPoolSize >= 1, "Invalid max pool size: " +
maxPoolSize);
+ return maxPoolSize;
+ }
+
+ public ExecutorConfig setMaxPoolSize(int maxPoolSize) {
Review 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](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadPoolExecutor.html)).
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.)
...
"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
