original-brownbear commented on PR #13609: URL: https://github.com/apache/lucene/pull/13609#issuecomment-2255530896
> I believe this change is ok, but I am no longer very confident about TestTaskExecutor#testCancelTasksOnException testing a real-life scenario, because it was recently changed to no longer rely on executorService. If I restore that, I get some failures here and there I don't think it was ever materially changed. Before it used to run on a single threaded executor and we'd block the main thread so it was effectively single threaded anyway? If you want to test that a task failure makes all tasks past it in the queue fail, you kinda need a single thread only for things to be deterministic? > (does not have to be perfect, but just something that verifies that the result is nullified? Hmm, it's quite hard to create a useful test here with concurrency enabled. But maybe we could assert that all tasks/runnables are either completed or have been started at the end of `setException`, i.e. after the cancelAll? -- 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
