uschindler commented on code in PR #12569:
URL: https://github.com/apache/lucene/pull/12569#discussion_r1331117098
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -22,18 +22,29 @@
import java.util.Collection;
import java.util.List;
import java.util.Objects;
+import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
import java.util.concurrent.Future;
-import java.util.concurrent.RunnableFuture;
+import java.util.concurrent.FutureTask;
import org.apache.lucene.util.IOUtils;
import org.apache.lucene.util.ThreadInterruptedException;
/**
* Executor wrapper responsible for the execution of concurrent tasks. Used to
parallelize search
- * across segments as well as query rewrite in some cases.
+ * across segments as well as query rewrite in some cases. Exposes a {@link
#createTask(Callable)}
+ * method to create tasks given a {@link Callable}, as well as the {@link
#invokeAll(Collection)}
+ * method to execute a set of tasks concurrently. Once all tasks are submitted
to the executor, it
+ * blocks and wait for all tasks to be completed, and then returns a list with
the obtained results.
+ * Ensures that the underlying executor is only used for top-level {@link
#invokeAll(Collection)}
+ * calls, and not for potential {@link #invokeAll(Collection)} calls made from
one of the tasks.
+ * This is to prevent deadlock with certain types of executors, as well as to
limit the level of
+ * parallelism.
*/
class TaskExecutor {
+ // a static thread local is ok as long as there is a single TaskExecutor
ever created
Review Comment:
Yes, static is fine.
You only have to be careful with multiple index searchers all using a single
ThreadPool instance (e.g., passed to many shards, each having their own
IndexSearcher). So multiple IndexSearchers would share same thread pool,
although all have a different instance of TaskExecutor.
To workaround possible inconsistencies, I suggest to use (it was already
implemented by @javanna) to increment/decrement instead of a binary switch,
because it could happen that a task could execute another IndexSearcher method
(e.g. also from another seracher of a different index, but using the same
thread pool, e.g. to execute a paralelizzed join between indexes). If we only
have a boolean instead of an integer, the state of the threadlocal after
returning from the inner call would be false (instead true), so another call to
s subjob would then parallize.
By using a counter (as suggested above and already implemented) we are safe
for such usages. Each task increments counter and decrements when done.
In addition a counter would help us easily to implement parallelism by
allowing the counter to go >1 before we switch to serial.
--
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]
