psalagnac commented on code in PR #4675:
URL: https://github.com/apache/solr/pull/4675#discussion_r3675955841
##########
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##########
@@ -245,36 +259,104 @@ public static ExecutorService
newMDCAwareCachedThreadPool(String name) {
* Create a new pool of threads, with no limit for the number of threads.
The pool has no task
* queue. Each submitted task is executed immediately, either by reusing an
existing thread if one
* is available, or by starting a new thread. Unused threads will be closed
after 60 seconds.
+ *
+ * <p>Thread count tracks how many tasks run concurrently, but nothing
bounds it: a burst of
+ * simultaneous tasks starts a thread apiece. Only use this where something
upstream already
+ * limits how many tasks can be in flight.
*/
public static ExecutorService newMDCAwareCachedThreadPool(ThreadFactory
threadFactory) {
Review Comment:
This method still takes a ThreadFactory.
Should it be deprecated in favor of the one taking a string?
##########
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##########
@@ -334,9 +419,41 @@ public MDCAwareThreadPoolExecutor(
BlockingQueue<Runnable> workQueue,
RejectedExecutionHandler handler) {
super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue,
handler);
+ checkPoolConfig(corePoolSize, maximumPoolSize, workQueue);
this.enableSubmitterStackTrace = true;
}
+ /**
+ * Rejects a pool that can never reach {@code maximumPoolSize}. {@link
ThreadPoolExecutor} only
Review Comment:
I don't think this statement is correct.
A queue with **unlimited** capacity caps the pool at corePoolSize, but even
a queue with a fixed capacity, like 1000 tasks, will allow the pool to grow wil
the queue is full.
That's a pattern that we may want to disallow, but that's not an invalid
pool config.
##########
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##########
@@ -245,36 +259,104 @@ public static ExecutorService
newMDCAwareCachedThreadPool(String name) {
* Create a new pool of threads, with no limit for the number of threads.
The pool has no task
* queue. Each submitted task is executed immediately, either by reusing an
existing thread if one
* is available, or by starting a new thread. Unused threads will be closed
after 60 seconds.
+ *
+ * <p>Thread count tracks how many tasks run concurrently, but nothing
bounds it: a burst of
+ * simultaneous tasks starts a thread apiece. Only use this where something
upstream already
+ * limits how many tasks can be in flight.
*/
public static ExecutorService newMDCAwareCachedThreadPool(ThreadFactory
threadFactory) {
return new MDCAwareThreadPoolExecutor(
0, Integer.MAX_VALUE, 60L, TimeUnit.SECONDS, new SynchronousQueue<>(),
threadFactory);
}
/**
- * Create a new pool of threads. Threads are created for new work if there
is room to do so up to
- * {@code maxThreads}. Beyond that, the queue is used up to {@code
queueCapacity}. Beyond that,
- * work is rejected with an exception. Unused threads will be closed after
60 seconds.
+ * Create a new pool of at most {@code nThreads} threads. Each submitted
task starts a thread
Review Comment:
> Each submitted task starts a thread until nThreads exist — even when
an idle thread could have taken it
Wow; I was really surprised by that so I wrote my own test to confirm this
statement...
Maybe we should revisit this. This does not make sense to me to create more
threads as long we have some available.
Why don't we always set the core pool size to 0 ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
