You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/06/04 18:20:38 UTC
[GitHub] [accumulo] ivakegg opened a new pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
ivakegg opened a new pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622
* Added properties to allow overridding the allowCoreThreadTimeout in
various threadpools: master incoming requests, tserver incoming requests, master bulk imports
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#issuecomment-654942794
Are you going to back port these changes to 1.10?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r444332545
##########
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:
I changed this to TIMEOUT_SECS with a value of 180. I am wondering whether this should be configurable as well. @keith-turner thoughts?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r448386243
##########
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:
Your idea is starting to grow on me. Instead of ALLOW_TIMEOUT properties I will create a THREAD_TIMEOUT properties which default to 0. I will work that through and update unless I find any issues.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r448626523
##########
File path: core/src/main/java/org/apache/accumulo/core/util/SimpleThreadPool.java
##########
@@ -28,15 +28,32 @@
*/
public class SimpleThreadPool extends ThreadPoolExecutor {
- public SimpleThreadPool(int max, final String name) {
- super(max, max, 4L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
+ // the number of seconds before we allow a thread to terminate with non-use.
+ public static final long DEFAULT_TIMEOUT_MILLISECS = 180000L;
+
+ public SimpleThreadPool(int coreAndMax, final String name) {
+ this(coreAndMax, DEFAULT_TIMEOUT_MILLISECS, name);
+ }
+
+ public SimpleThreadPool(int coreAndMax, long threadTimeOut, final String name) {
+ super(coreAndMax, coreAndMax, threadTimeOut, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(),
Review comment:
Would it work to only do the following in the constructor?
```suggestion
this(coreAndMax, threadTimeOut, name, new LinkedBlockingQueue<>());
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r450850855
##########
File path: core/src/main/java/org/apache/accumulo/core/util/SimpleThreadPool.java
##########
@@ -28,15 +28,32 @@
*/
public class SimpleThreadPool extends ThreadPoolExecutor {
- public SimpleThreadPool(int max, final String name) {
- super(max, max, 4L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
+ // the number of seconds before we allow a thread to terminate with non-use.
+ public static final long DEFAULT_TIMEOUT_MILLISECS = 180000L;
+
+ public SimpleThreadPool(int coreAndMax, final String name) {
+ this(coreAndMax, DEFAULT_TIMEOUT_MILLISECS, name);
+ }
+
+ public SimpleThreadPool(int coreAndMax, long threadTimeOut, final String name) {
+ super(coreAndMax, coreAndMax, threadTimeOut, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(),
Review comment:
done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#issuecomment-654923546
shoot, I meant to squash the commits. I screwed that one up. sorry folks
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r444336254
##########
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",
+ PropertyType.BOOLEAN,
+ "True if the incoming request threads are allowed to timeout with no work available."),
Review comment:
I have no problem changing the descriptions as such. done.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] phrocker commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
phrocker commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r440832957
##########
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",
+ PropertyType.BOOLEAN,
+ "True if the incoming request threads are allowed to timeout with no work available."),
Review comment:
minor nit: I think this is a great set of features; however, I had a question about the wording. "True... if... allowed" made me think another setting would impact this, but I imagine it is the case that if this is true then timeouts can occur.
"Allows request threads to time out when no work is available" might be more clear but @keith-turner spent more time here so he can likely correct me from his perspective.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r448632279
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -231,8 +236,8 @@ public static ServerAddress startServer(MetricsSystem metricsSystem, ServerConte
*/
public static ServerAddress createThreadedSelectorServer(HostAndPort address,
TProcessor processor, TProtocolFactory protocolFactory, final String serverName,
- final int numThreads, final int numSTThreads, long timeBetweenThreadChecks,
- long maxMessageSize) throws TTransportException {
+ final int numThreads, final long threadTimeOut, final int numSTThreads,
Review comment:
I was trying to ensure the parameter order was correct when looking at this PR. Once I realized you had consistently put the timeout after the number of threads here and elsewhere it made verifying the order much easier.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r450828405
##########
File path: core/src/main/java/org/apache/accumulo/core/util/SimpleThreadPool.java
##########
@@ -28,15 +28,32 @@
*/
public class SimpleThreadPool extends ThreadPoolExecutor {
- public SimpleThreadPool(int max, final String name) {
- super(max, max, 4L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
+ // the number of seconds before we allow a thread to terminate with non-use.
+ public static final long DEFAULT_TIMEOUT_MILLISECS = 180000L;
+
+ public SimpleThreadPool(int coreAndMax, final String name) {
+ this(coreAndMax, DEFAULT_TIMEOUT_MILLISECS, name);
+ }
+
+ public SimpleThreadPool(int coreAndMax, long threadTimeOut, final String name) {
+ super(coreAndMax, coreAndMax, threadTimeOut, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(),
Review comment:
That would work and would be cleaner. I could also them remove the redundant lines after. I will add this change.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r435894344
##########
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", "true",
Review comment:
ok, I am game for that. I will change it now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#issuecomment-639458877
Any thoughts on whether this should be done on the 1.9 branch? I was hesitant since we are adding new properties, however I know our system will want this in 1.9. I can always back-port on our internal branch.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] phrocker commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
phrocker commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r440832957
##########
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",
+ PropertyType.BOOLEAN,
+ "True if the incoming request threads are allowed to timeout with no work available."),
Review comment:
minor nit: I think this is a great set of features; however, I had a question about the wording. "True... if... allowed" made me think another setting would impact this, but I imagine it is the case that if this is true then timeouts can occur.
"Allows request threads to time out when no work is available" might be more clear but @keith-turner spent more time here so he can likely correct me from his perspective -- and it may be the case that my interpretation is wrong.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg merged pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg merged pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r444534289
##########
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:
I am not sure it needs to be made configurable. If it were configurable maybe we could have a single property with just the timeout and a timeout of zero means no timeout.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#issuecomment-665397559
@ivakegg Please don't forget to set the project version on issues/PRs at some point before they are closed, so they show up on the project planning board for that version (in this case, it shows up at https://github.com/apache/accumulo/projects/3#card-42711125 after I updated this to add the project).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r435507270
##########
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", "true",
Review comment:
I thought about that. I think this only really becomes a problem when you are dealing with very large numbers of threads (300+ maybe) which is why I left the default as true. I am certainly will to change it to false however.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r435512348
##########
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", "true",
Review comment:
I am slightly in favor of false because it addresses a known problem and seems like it should not cause problems, but it could. The way this change is structured it does not change all of the existing code to false just a few select places, which is good. Leaving most of the internal usage w/ a hard coded true is good and changing just these configurable ones to false may be a good way to go.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#issuecomment-652610767
> LGTM. Good find!
@brianloss @keith-turner I apologize, but changing the mechanism from a boolean:allow_timeout to a long:timeout had enough change that I need you to take a pass through this again. Thank you for your time!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r435467765
##########
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", "true",
Review comment:
The default of true maintains the current behavior, however that behavior seemed problematic in the issue you opened. Makes me wonder if the default should be false.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r450851005
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -231,8 +236,8 @@ public static ServerAddress startServer(MetricsSystem metricsSystem, ServerConte
*/
public static ServerAddress createThreadedSelectorServer(HostAndPort address,
TProcessor processor, TProtocolFactory protocolFactory, final String serverName,
- final int numThreads, final int numSTThreads, long timeBetweenThreadChecks,
- long maxMessageSize) throws TTransportException {
+ final int numThreads, final long threadTimeOut, final int numSTThreads,
Review comment:
cool
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1622: fixes #1621: The ClientPool thread pool allows all core threads to time out
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1622:
URL: https://github.com/apache/accumulo/pull/1622#discussion_r448632279
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -231,8 +236,8 @@ public static ServerAddress startServer(MetricsSystem metricsSystem, ServerConte
*/
public static ServerAddress createThreadedSelectorServer(HostAndPort address,
TProcessor processor, TProtocolFactory protocolFactory, final String serverName,
- final int numThreads, final int numSTThreads, long timeBetweenThreadChecks,
- long maxMessageSize) throws TTransportException {
+ final int numThreads, final long threadTimeOut, final int numSTThreads,
Review comment:
I was trying to ensure the parameter order was correct when looking at this. Once I realized you had consistently put the timeout after the number of threads here and elsewhere it made verify the order much easier.
----------------------------------------------------------------
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:
users@infra.apache.org