virajjasani commented on a change in pull request #2922:
URL: https://github.com/apache/hbase/pull/2922#discussion_r572088450
##########
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:
> delegate them to ThreadPoolExecutor, it checks all these anyway
That's better. And perhaps we can just comment here that ThreadPoolExecutor
can throw IllegalArgumentException if one of the following holds:
```
* {@code corePoolSize < 0}<br>
* {@code keepAliveTime < 0}<br>
* {@code maximumPoolSize <= 0}<br>
* {@code maximumPoolSize < corePoolSize}
```
or maybe just point to Javadoc of ThreadPoolExecutor constructor, anything
that you feel comfortable.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
##########
@@ -81,15 +82,16 @@ public ExecutorService(final String servername) {
/**
* Start an executor service with a given name. If there was a service
already
* started with the same name, this throws a RuntimeException.
- * @param name Name of the service to start.
+ * @param config Configuration to use for the executor.
*/
- public void startExecutorService(String name, int maxThreads) {
+ public void startExecutorService(final ExecutorConfig config) {
Review comment:
Javadoc of this class seems to be referring to deprecated usage:
```
* <p>In order to create a new service, create an instance of this class and
* then do: <code>instance.startExecutorService("myService");</code>. When
done
* call {@link #shutdown()}.
```
Good to change that too with this PR?
##########
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:
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!
----------------------------------------------------------------
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
