dsmiley commented on code in PR #4675:
URL: https://github.com/apache/solr/pull/4675#discussion_r3658664926
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
Review Comment:
many changes in this PR are just for the rename of one
`newMDCAwareCachedThreadPool` to `newMDCAwareFixedThreadPool`
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -109,7 +109,7 @@ public void close() {
ExecutorService coreCloseExecutor =
ExecutorUtil.newMDCAwareFixedThreadPool(
- Integer.MAX_VALUE, new
SolrNamedThreadFactory("coreCloseExecutor"));
+ 64, new SolrNamedThreadFactory("coreCloseExecutor"));
Review Comment:
new enforcement caught this problem
##########
solr/core/src/test/org/apache/solr/request/TestUnInvertedFieldException.java:
##########
Review Comment:
checks caught this
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##########
Review Comment:
Very much deserves to be a separate PR, see #4668
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
Review Comment:
Very much deserves a separate PR, see #4655 or could be combined with the
Jetty one.
##########
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
+ * creates threads beyond {@code corePoolSize} once the queue is full, so
a queue with spare
+ * capacity silently caps the pool at {@code corePoolSize} (or at one
thread, which is always
+ * created to rescue a queued task when {@code corePoolSize} is 0).
+ */
+ private static void checkPoolConfig(
Review Comment:
see the rules being added
##########
solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java:
##########
@@ -1419,6 +1404,10 @@ public void checkExpectedSchemaField(Map<String, Object>
expected) throws Except
assertEquals("Field: " + fieldName, expected, rsp.getField());
}
+ private ExecutorService newThreadPerUpdatePool(List<?> updates) {
+ return ExecutorUtil.newMDCAwareFixedThreadPool(updates.size() + 1, 1,
getTestName());
Review Comment:
1 queue size... we don't expect to use the queue at all in this test; this
is a glorified thread-per-task generator
##########
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##########
@@ -210,7 +216,15 @@ public static void awaitTerminationForever(ExecutorService
pool) {
}
}
- /** See {@link java.util.concurrent.Executors#newFixedThreadPool(int,
ThreadFactory)} */
+ /**
+ * See {@link java.util.concurrent.Executors#newFixedThreadPool(int,
ThreadFactory)}. The queue is
+ * unbounded, so work beyond {@code nThreads} is never rejected; it
accumulates in memory instead.
+ * Use {@link #newMDCAwareFixedThreadPool(int, int, String)} to bound it
— note that
+ * overload also reclaims idle threads, whereas these live as long as the
pool.
+ *
+ * <p>This is good for fixed-size workloads, like for heavy/intensive work.
+ */
+ @Deprecated(since = "10.1") // prefer the overloaded one, thus explicit
about queue capacity
Review Comment:
Wanting to see this deprecated because I think the queue capacity should be
more visible at the call-site, surfacing a trade-off/choice. Also, this method
doesn't do the 60 second core pool size reclamation, which seems to me a
universally good thing. I'm looking for feedback here. If there are some
cases that don't want idle reclaimation, they can easily toggle that aspect
after pool creation.
##########
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
+ * until {@code nThreads} exist — even when an idle thread could have
taken it — so a
+ * pool that sees many tasks reaches {@code nThreads} regardless of how many
run at once. 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.
+ *
+ * <p>This is good for limiting heavy/intensive workloads, and that which
need to reject tasks
+ * when the queue is full.
*/
- public static ExecutorService newMDCAwareCachedThreadPool(
- int maxThreads, int queueCapacity, ThreadFactory threadFactory) {
- // Create an executor with same value of core size and max total size.
With an unbounded queue,
- // the ThreadPoolExecutor ignores the configured max value and only
considers core pool size.
- // Since we allow core threads to die when idle for too long, this ends in
having a pool with
- // lazily-initialized and cached threads.
+ public static ExecutorService newMDCAwareFixedThreadPool(
Review Comment:
renamed. And using a String pool name since *every* caller is using
SolrNamedThreadFactory. Many years ago, there was a thread factory split that
doesn't exist today.
##########
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
+ * until {@code nThreads} exist — even when an idle thread could have
taken it — so a
+ * pool that sees many tasks reaches {@code nThreads} regardless of how many
run at once. 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.
+ *
+ * <p>This is good for limiting heavy/intensive workloads, and that which
need to reject tasks
+ * when the queue is full.
*/
- public static ExecutorService newMDCAwareCachedThreadPool(
- int maxThreads, int queueCapacity, ThreadFactory threadFactory) {
- // Create an executor with same value of core size and max total size.
With an unbounded queue,
- // the ThreadPoolExecutor ignores the configured max value and only
considers core pool size.
- // Since we allow core threads to die when idle for too long, this ends in
having a pool with
- // lazily-initialized and cached threads.
+ public static ExecutorService newMDCAwareFixedThreadPool(
+ int nThreads, int queueCapacity, String poolName) {
+ if (nThreads > 1024) { // likely wrong choice
Review Comment:
adding a check
--
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]
