javanna commented on code in PR #12569:
URL: https://github.com/apache/lucene/pull/12569#discussion_r1330223982
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -22,18 +22,28 @@
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 {
+ private static final ThreadLocal<Boolean> isConcurrentTask =
ThreadLocal.withInitial(() -> false);
Review Comment:
I am aware of some of the subtleties of thread locals. I started with a
solution that did not use them but all in all it would do the same that thread
locals do :) (e.g. keeping track of which thread runs what in a map).
I think that if we make sure that a single task executor is created, which
is currently the case, we should be ok?
I am happy to address the naming as suggested, I was also not super happy
with it.
I will play with the counter idea, though I don't think that allowing
multiple levels of parallelism is required? Is that necessary in your opinion?
> Would it not be better to pass around the Task instance and the task
instance has a method to spawn a subtask?
I spent quite some time debating this with myself as well. The problem is
that invokeAll does not have the current task. It may or may not be executed as
part of a task. It looks like making the task available (without using thread
locals!) would mean carrying the task around in many places, unless I am
missing another way.
Thanks for all the feedback Uwe!
--
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: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
