javanna commented on code in PR #13472:
URL: https://github.com/apache/lucene/pull/13472#discussion_r1664308783
##########
lucene/core/src/test/org/apache/lucene/search/TestTaskExecutor.java:
##########
@@ -308,7 +306,7 @@ public void testInvokeAllCatchesMultipleExceptions() {
}
public void testCancelTasksOnException() {
- TaskExecutor taskExecutor = new TaskExecutor(executorService);
+ TaskExecutor taskExecutor = new TaskExecutor(Runnable::run);
Review Comment:
yes, thanks.
##########
lucene/core/src/test/org/apache/lucene/search/TestTaskExecutor.java:
##########
@@ -251,14 +271,15 @@ public void testInvokeAllDoesNotLeaveTasksBehind() {
for (int i = 0; i < tasksWithNormalExit; i++) {
callables.add(
() -> {
- tasksExecuted.incrementAndGet();
- return null;
+ throw new AssertionError(
+ "must not be called since the first task failing cancels all
subsequent tasks");
});
}
expectThrows(RuntimeException.class, () ->
taskExecutor.invokeAll(callables));
assertEquals(1, tasksExecuted.get());
// the callables are technically all run, but the cancelled ones will be
no-op
- assertEquals(100, tasksStarted.get());
+ // add one for the task the gets executed on the current thread
Review Comment:
this comment is now outdated? I do find the assert below now confusing,
perhaps it does need a comment to explain that the `-1` is not because one task
failed, but rather because the last task gets executed by the caller thread?
I had asked if we can check that the task that gets executed by the caller
thread is skipped when the first task throws an exception. Is that possible?
##########
lucene/CHANGES.txt:
##########
@@ -277,6 +277,15 @@ Optimizations
* GITHUB#12941: Don't preserve auxiliary buffer contents in LSBRadixSorter if
it grows. (Stefan Vodita)
+Changes in runtime behavior
+---------------------
+
+* GITHUB#13472: IndexSearcher now executes tasks on the thread that invoked a
search as well as its configured
+ executor. Users that invoke searches from threads not belonging to the
executor configured in IndexSearcher should
+ reduce the executor's thread-count by 1 to retain the previous level of
parallelism. Searches invoked from a thread
+ not part of the IndexSearcher's executor will now complete on the calling
thread in case the executor is busy or
+ blocked and unable to complete any work.
Review Comment:
Thanks for writing this up. Some comments on the text:
I think that we should provide context: "When an executor is provided to the
IndexSearcher constructor, the searcher now executes tasks ....." , and perhaps
highlight the change "while previously all of the computation was offloaded to
the executor."
I wonder if there's any user that calls search from a thread of the executor
so far. I think that a separate executor was required until now, although we
did not write that as a specific requirement anywhere? We would deadlock
otherwise?
Should we be explicitly indicating that it is now possible to call search
from a thread of the executor, and a separate executor for parallel execution
is no longer required?
--
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]
