keith-turner commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r436854946
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -259,6 +262,9 @@
"Regular expression that defines the set of Tablet Servers that will
perform bulk imports"),
MASTER_MINTHREADS("master.server.threads.minimum", "20", PropertyType.COUNT,
"The minimum number of threads to use to handle incoming requests."),
+ MASTER_MINTHREADS_ALLOW_TIMEOUT("master.server.thread.timeout.allowed",
"false",
Review comment:
```suggestion
MASTER_MINTHREADS_ALLOW_TIMEOUT("master.server.threads.timeout.allowed",
"false",
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -452,6 +458,9 @@
"The time to wait for a tablet server to process a bulk import
request."),
TSERV_MINTHREADS("tserver.server.threads.minimum", "20", PropertyType.COUNT,
"The minimum number of threads to use to handle incoming requests."),
+ TSERV_MINTHREADS_ALLOW_TIMEOUT("tserver.server.thread.timeout.allowed",
"false",
Review comment:
```suggestion
TSERV_MINTHREADS_ALLOW_TIMEOUT("tserver.server.threads.timeout.allowed",
"false",
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -246,6 +246,9 @@
"The number of attempts to bulk import a RFile before giving up."),
MASTER_BULK_THREADPOOL_SIZE("master.bulk.threadpool.size", "5",
PropertyType.COUNT,
"The number of threads to use when coordinating a bulk import."),
+ MASTER_BULK_THREADPOOL_ALLOW_TIMEOUT("master.bulk.thread.timeout.allowed",
"false",
Review comment:
To me making the property perfix the same as far as possible with the
existing prop provides a clue that these two props go together.
```suggestion
MASTER_BULK_THREADPOOL_ALLOW_TIMEOUT("master.bulk.threadpool.timeout.allowed",
"false",
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/SimpleThreadPool.java
##########
@@ -28,15 +28,16 @@
*/
public class SimpleThreadPool extends ThreadPoolExecutor {
- public SimpleThreadPool(int max, final String name) {
- super(max, max, 4L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
+ public SimpleThreadPool(int coreAndMax, boolean allowCoreThreadTimeOut,
final String name) {
+ super(coreAndMax, coreAndMax, 4L, TimeUnit.SECONDS, new
LinkedBlockingQueue<>(),
Review comment:
```suggestion
super(coreAndMax, coreAndMax, TIMEOUT_SECS, TimeUnit.SECONDS, new
LinkedBlockingQueue<>(),
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/SimpleThreadPool.java
##########
@@ -28,15 +28,16 @@
*/
public class SimpleThreadPool extends ThreadPoolExecutor {
- public SimpleThreadPool(int max, final String name) {
- super(max, max, 4L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
+ public SimpleThreadPool(int coreAndMax, boolean allowCoreThreadTimeOut,
final String name) {
+ super(coreAndMax, coreAndMax, 4L, TimeUnit.SECONDS, new
LinkedBlockingQueue<>(),
new NamingThreadFactory(name));
- allowCoreThreadTimeOut(true);
+ allowCoreThreadTimeOut(allowCoreThreadTimeOut);
}
- public SimpleThreadPool(int max, final String name, BlockingQueue<Runnable>
queue) {
- super(max, max, 4L, TimeUnit.SECONDS, queue, new
NamingThreadFactory(name));
- allowCoreThreadTimeOut(true);
+ public SimpleThreadPool(int coreAndMax, boolean allowCoreThreadTimeOut,
final String name,
+ BlockingQueue<Runnable> queue) {
+ super(coreAndMax, coreAndMax, 4L, TimeUnit.SECONDS, queue, new
NamingThreadFactory(name));
Review comment:
While we examining this code, I think it would be a good idea to
increase this timeout to a few minutes and use a a private constant. Maybe 3
to 5 mins. Could do `private static final TIMEOUT_SECS=180`
```suggestion
super(coreAndMax, coreAndMax, TIMEOUT_SECS, TimeUnit.SECONDS, queue, new
NamingThreadFactory(name));
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/SimpleThreadPool.java
##########
@@ -28,15 +28,16 @@
*/
public class SimpleThreadPool extends ThreadPoolExecutor {
- public SimpleThreadPool(int max, final String name) {
- super(max, max, 4L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
+ public SimpleThreadPool(int coreAndMax, boolean allowCoreThreadTimeOut,
final String name) {
+ super(coreAndMax, coreAndMax, 4L, TimeUnit.SECONDS, new
LinkedBlockingQueue<>(),
new NamingThreadFactory(name));
- allowCoreThreadTimeOut(true);
+ allowCoreThreadTimeOut(allowCoreThreadTimeOut);
}
- public SimpleThreadPool(int max, final String name, BlockingQueue<Runnable>
queue) {
- super(max, max, 4L, TimeUnit.SECONDS, queue, new
NamingThreadFactory(name));
- allowCoreThreadTimeOut(true);
+ public SimpleThreadPool(int coreAndMax, boolean allowCoreThreadTimeOut,
final String name,
+ BlockingQueue<Runnable> queue) {
+ super(coreAndMax, coreAndMax, 4L, TimeUnit.SECONDS, queue, new
NamingThreadFactory(name));
Review comment:
While we are examining this code, I think it would be a good idea to
increase this timeout to a few minutes and use a a private constant. Maybe 3
to 5 mins. Could do `private static final TIMEOUT_SECS=180`
```suggestion
super(coreAndMax, coreAndMax, TIMEOUT_SECS, TimeUnit.SECONDS, queue, new
NamingThreadFactory(name));
```
----------------------------------------------------------------
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:
[email protected]
