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 2021/09/20 20:16:39 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
dlmarion opened a new pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278
Closes #2272
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r712558030
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
+ fatePoolWatcher.schedule(() -> {
+ ThreadPools.makeResizeable(pool, conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
+ int remaining = 1000 - pool.getQueue().size();
Review comment:
Could replace 1000 w/ a function of the numThreads and time like below.
```suggestion
// Assume a thread could execute up to 100 operations a second. We are sleeping for three seconds. So to calculate max to queue do 3* 100 * numThreads.
int maxToQueue = 300 * conf.getCount( Property.MANAGER_FATE_THREADPOOL_SIZE);
int remaining = maxToQueue - pool.getQueue().size();
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713171133
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
hmm so maybe this code never achieved its intent.. I wonder if it only increased but never decreased
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r712977220
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -29,33 +29,30 @@
import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class ThreadPools {
+ private static final Logger LOG = LoggerFactory.getLogger(ThreadPools.class);
+
// the number of seconds before we allow a thread to terminate with non-use.
public static final long DEFAULT_TIMEOUT_MILLISECS = 180000L;
- private static void makeResizeable(final ThreadPoolExecutor pool,
- final AccumuloConfiguration conf, final Property p) {
- final String threadName = p.name().concat("_watcher");
- Threads.createThread(threadName, () -> {
- int count = conf.getCount(p);
- while (Thread.currentThread().isAlive() && !Thread.currentThread().isInterrupted()) {
- try {
- Thread.sleep(1000);
- int newCount = conf.getCount(p);
- if (newCount != count) {
- pool.setCorePoolSize(newCount);
- pool.setMaximumPoolSize(newCount);
- count = newCount;
- }
- } catch (InterruptedException e) {
- // throw a RuntimeException and let the AccumuloUncaughtExceptionHandler deal with it.
- throw new RuntimeException("Thread " + threadName + " was interrupted.");
- }
- }
- }).start();
-
+ public static void makeResizeable(final ThreadPoolExecutor pool, final AccumuloConfiguration conf,
Review comment:
I included this in 8bed9e5.
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -29,33 +29,30 @@
import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class ThreadPools {
+ private static final Logger LOG = LoggerFactory.getLogger(ThreadPools.class);
+
// the number of seconds before we allow a thread to terminate with non-use.
public static final long DEFAULT_TIMEOUT_MILLISECS = 180000L;
- private static void makeResizeable(final ThreadPoolExecutor pool,
- final AccumuloConfiguration conf, final Property p) {
- final String threadName = p.name().concat("_watcher");
- Threads.createThread(threadName, () -> {
- int count = conf.getCount(p);
- while (Thread.currentThread().isAlive() && !Thread.currentThread().isInterrupted()) {
- try {
- Thread.sleep(1000);
- int newCount = conf.getCount(p);
- if (newCount != count) {
- pool.setCorePoolSize(newCount);
- pool.setMaximumPoolSize(newCount);
- count = newCount;
- }
- } catch (InterruptedException e) {
- // throw a RuntimeException and let the AccumuloUncaughtExceptionHandler deal with it.
- throw new RuntimeException("Thread " + threadName + " was interrupted.");
- }
- }
- }).start();
-
+ public static void makeResizeable(final ThreadPoolExecutor pool, final AccumuloConfiguration conf,
+ final Property p) {
+ int count = pool.getMaximumPoolSize();
+ int newCount = conf.getCount(p);
Review comment:
I included this in 8bed9e5.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713163570
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
So, it would [appear](https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/master/src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java#L1533) that raising the corePoolSize above the MaxPoolSize throws an error. Maybe this is what you were referring to?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713213205
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
> It is used by the TabletServerBatchWriter. I think putting it on the ServerContext would render that unreachable.
May make sense to have it in the client context for client side code. Server context extends client context, so maybe that would work out nicely.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713203801
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
It is used by the TabletServerBatchWriter. I think putting it on the ServerContext would render that unreachable.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r712977751
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
+ fatePoolWatcher.schedule(() -> {
+ ThreadPools.makeResizeable(pool, conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
+ int remaining = 1000 - pool.getQueue().size();
Review comment:
I included this in 8bed9e5.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713183296
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
Looking back at 1.10 I See the following which created a singleton timer that was shared in the process with a configurable number of threads.
https://github.com/apache/accumulo/blob/bbe2de8a8c00eb0b7cc01766ac6e6db672bf8444/server/base/src/main/java/org/apache/accumulo/server/util/time/SimpleTimer.java#L60-L71
AFAICT now in 2.1.0 there is no longer a process wide singleton timer related to the config `general.server.simpletimer.threadpool.size`?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r712548893
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -29,33 +29,30 @@
import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class ThreadPools {
+ private static final Logger LOG = LoggerFactory.getLogger(ThreadPools.class);
+
// the number of seconds before we allow a thread to terminate with non-use.
public static final long DEFAULT_TIMEOUT_MILLISECS = 180000L;
- private static void makeResizeable(final ThreadPoolExecutor pool,
- final AccumuloConfiguration conf, final Property p) {
- final String threadName = p.name().concat("_watcher");
- Threads.createThread(threadName, () -> {
- int count = conf.getCount(p);
- while (Thread.currentThread().isAlive() && !Thread.currentThread().isInterrupted()) {
- try {
- Thread.sleep(1000);
- int newCount = conf.getCount(p);
- if (newCount != count) {
- pool.setCorePoolSize(newCount);
- pool.setMaximumPoolSize(newCount);
- count = newCount;
- }
- } catch (InterruptedException e) {
- // throw a RuntimeException and let the AccumuloUncaughtExceptionHandler deal with it.
- throw new RuntimeException("Thread " + threadName + " was interrupted.");
- }
- }
- }).start();
-
+ public static void makeResizeable(final ThreadPoolExecutor pool, final AccumuloConfiguration conf,
Review comment:
This method name seems off w/ the changes.
```suggestion
public static void resizePool(final ThreadPoolExecutor pool, final AccumuloConfiguration conf,
```
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
The Accumulo servers used to have a shared timer pool. Not sure if that is still a thing, if it is could be used to avoid creating these watcher threads.
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -29,33 +29,30 @@
import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class ThreadPools {
+ private static final Logger LOG = LoggerFactory.getLogger(ThreadPools.class);
+
// the number of seconds before we allow a thread to terminate with non-use.
public static final long DEFAULT_TIMEOUT_MILLISECS = 180000L;
- private static void makeResizeable(final ThreadPoolExecutor pool,
- final AccumuloConfiguration conf, final Property p) {
- final String threadName = p.name().concat("_watcher");
- Threads.createThread(threadName, () -> {
- int count = conf.getCount(p);
- while (Thread.currentThread().isAlive() && !Thread.currentThread().isInterrupted()) {
- try {
- Thread.sleep(1000);
- int newCount = conf.getCount(p);
- if (newCount != count) {
- pool.setCorePoolSize(newCount);
- pool.setMaximumPoolSize(newCount);
- count = newCount;
- }
- } catch (InterruptedException e) {
- // throw a RuntimeException and let the AccumuloUncaughtExceptionHandler deal with it.
- throw new RuntimeException("Thread " + threadName + " was interrupted.");
- }
- }
- }).start();
-
+ public static void makeResizeable(final ThreadPoolExecutor pool, final AccumuloConfiguration conf,
+ final Property p) {
+ int count = pool.getMaximumPoolSize();
+ int newCount = conf.getCount(p);
Review comment:
Thinking the following may avoid logging when there is nothing to do.
```suggestion
int newCount = conf.getCount(p);
if(count == newCount)
return;
```
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
+ fatePoolWatcher.schedule(() -> {
+ ThreadPools.makeResizeable(pool, conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
+ int remaining = 1000 - pool.getQueue().size();
+ for (int i = 0; i < remaining; i++) {
+ pool.execute(new TransactionRunner());
Review comment:
This may throw a bunch of exceptions if the pool is shutdown. Would be nice to avoid that. Could wait for `fatePoolWatcher` to shutdown before calling `executor.shutdown()` in the shutdown() method below in this class. That might avoid the exceptions here.
Alternatively could catch rejectedexecutionexception here and ignore it if the pool is shutdown.
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
+ fatePoolWatcher.schedule(() -> {
+ ThreadPools.makeResizeable(pool, conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
+ int remaining = 1000 - pool.getQueue().size();
Review comment:
Could replace 1000 w/ a function of the numThreads and time like below.
```suggestion
// Assume a thread could execute up to 100 operations a second. We are sleeping for three seconds. So to calculate max to queue do 3* 100 * numThreads.
int maxToQueue = 300 * conf.getCount( Property.MANAGER_FATE_THREADPOOL_SIZE);
int remaining = 1000 - pool.getQueue().size();
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713208899
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
I might be able to put it on AccumuloConfiguration.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713172844
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
I can't say about before my change, but in ThreadPools.java as it is now, if the thread pool was created with a pool size of 5 and no max pool specified, then the max pool size would also be set to 5. If the core pool size was then decreased, it would error.
Edit: I think the new way is the correct way at this point.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713163570
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
So, it would [appear](https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/master/src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java#L1533) that lowering the corePoolSize below the MaxPoolSize throws an error. Maybe this is what you were referring to?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713172844
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
I can't say about before my change, but in ThreadPools.java as it is now, if the thread pool was created with a pool size of 5 and no max pool specified, then the max pool size would also be set to 5. If the core pool size was then decreased, it would error.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r712977529
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
+ fatePoolWatcher.schedule(() -> {
+ ThreadPools.makeResizeable(pool, conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
+ int remaining = 1000 - pool.getQueue().size();
+ for (int i = 0; i < remaining; i++) {
+ pool.execute(new TransactionRunner());
Review comment:
I addressed this in 8bed9e5.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713158377
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +224,26 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
+ fatePoolWatcher.schedule(() -> {
+ ThreadPools.resizePool(pool, conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
+ // Assume a thread could execute up to 100 operations a second.
+ // We are sleeping for three seconds. So to calculate max to queue do 3* 100 * numThreads.
+ int maxToQueue = 300 * conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
+ int remaining = maxToQueue - pool.getQueue().size();
+ for (int i = 0; i < remaining; i++) {
+ try {
+ pool.execute(new TransactionRunner());
+ } catch (RejectedExecutionException e) {
+ // RejectedExecutionException could be shutting down
+ log.warn("Error adding transaction runner to FaTE executor pool.");
Review comment:
I think if its shutdown we can avoid spamming the logs. If its not shutdown, then its really bad and we should include the exception. For the case where its not shutdown, should we log an error or rethrow the exception?
```suggestion
if(pool.isShutdown()){
// The exception is expected in this case, no need to spam the logs.
log.trace("Error adding transaction runner to FaTE executor pool.",e);
} else {
// This is bad, FaTE may no longer work!
log.error("Error adding transaction runner to FaTE executor pool.",e);
}
```
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
I have not looked at the context to see if this matters, I noticed this went from only calling `pool.setCorePoolSize()` to calling `pool.setMaximumPoolSize(...)` and `pool.setCorePoolSize()`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713168822
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
I just noticed that this code was no longer making the same method calls on the thread pool that it used to make, however I was not sure if that change mattered.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713169933
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
Right, it's now using the common way of doing the resizing.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713198374
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
Also it would be nice to avoid the static singelton and instead hang the shared timer pool off of the server context. However I suspect the server context path would be difficult. Could do a static singleton to quickly restore the behavior and then open an issue to move the static singleton to the server context.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r712949095
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
+ fatePoolWatcher.schedule(() -> {
+ ThreadPools.makeResizeable(pool, conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
+ int remaining = 1000 - pool.getQueue().size();
Review comment:
Thanks for this, I wasn't sure what a reasonable value would be.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713187169
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
That was my concern, that maybe I missed that. However, now that the creation of that ThreadPool is singularly located in `ThreadPools.createGeneralScheduledExecutorService`, it should be easy to make it a singleton that is only created if it does not already exist. It's called from 33 locations across the code base.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713171133
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
hmm so maybe this code (before this change) never achieved its intent.. I wonder if it only increased but never decreased
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713169459
##########
File path: server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
##########
@@ -323,16 +323,11 @@ public static ThreadPoolExecutor createSelfResizingThreadPool(final String serve
// however, this isn't really an issue, since it adjusts periodically anyway
if (pool.getCorePoolSize() <= pool.getActiveCount()) {
int larger = pool.getCorePoolSize() + Math.min(pool.getQueue().size(), 2);
- log.info("Increasing server thread pool size on {} to {}", serverName, larger);
- pool.setMaximumPoolSize(larger);
- pool.setCorePoolSize(larger);
+ ThreadPools.resizePool(pool, () -> larger, serverName + "-ClientPool");
} else {
if (pool.getCorePoolSize() > pool.getActiveCount() + 3) {
int smaller = Math.max(executorThreads, pool.getCorePoolSize() - 1);
- if (smaller != pool.getCorePoolSize()) {
- log.info("Decreasing server thread pool size on {} to {}", serverName, smaller);
- pool.setCorePoolSize(smaller);
- }
+ ThreadPools.resizePool(pool, () -> smaller, serverName + "-ClientPool");
Review comment:
Do you think the existing code before your change had a bug?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#issuecomment-931476197
I think this change still might be a problem. It is causing some of our previously stable ITs to timeout. I am not sure why but they were fine before the initial commit of https://github.com/apache/accumulo/pull/2274. The two ITs timing out are MergeIT and ShellServerIT
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713190992
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
I think it would be good to preserve that behavior (in another PR). Less to look at when looking at jstacks.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r712947526
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
I looked back in 1.10 for shared SimpleTimer's. Please let me know if you happen to have a location on this as I would be interested to see if I missed something when I did the Threads/ThreadPools work. In this specific case, only 1 scheduled runnable is going to be created as `startTransactionRunners` is only called once in Manager.run().
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2278: Resize FaTE thread pool correctly, attempt at making sure it's constantly running
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2278:
URL: https://github.com/apache/accumulo/pull/2278#discussion_r713192833
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -226,11 +223,17 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
- int numThreads = conf.getCount(Property.MANAGER_FATE_THREADPOOL_SIZE);
- executor = ThreadPools.createExecutorService(conf, Property.MANAGER_FATE_THREADPOOL_SIZE);
- for (int i = 0; i < numThreads; i++) {
- executor.execute(new TransactionRunner());
- }
+ final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
+ Property.MANAGER_FATE_THREADPOOL_SIZE);
+ fatePoolWatcher = ThreadPools.createGeneralScheduledExecutorService(conf);
Review comment:
I will open a ticket for it. Thanks.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org