Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TaskExecutor waits for all tasks to complete before returning #12523

Merged
merged 11 commits into from
Oct 5, 2023

Conversation

quux00
Copy link
Contributor

@quux00 quux00 commented Aug 28, 2023

The TaskExecutor used by IndexSearcher and AbstractKnnVectorQuery
waits until all tasks have finished before returning, even when
one or more of the tasks throw an Exception.

Relates #12278

@javanna javanna self-requested a review September 5, 2023 07:05
Copy link
Contributor

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @quux00 ! The fix looks good to me, I think we can improve on the testing side, I left a comment around that.

@quux00 quux00 force-pushed the concurrent-search/wait-on-all-tasks branch from a811d0e to 51d47e5 Compare September 28, 2023 20:37
@quux00 quux00 force-pushed the concurrent-search/wait-on-all-tasks branch from 51d47e5 to ba5d115 Compare October 2, 2023 13:29
Copy link
Contributor

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some nits on the testing part but it looks good already. Thanks!

@quux00 quux00 force-pushed the concurrent-search/wait-on-all-tasks branch from d9eb713 to 3ed2489 Compare October 3, 2023 16:00
@javanna
Copy link
Contributor

javanna commented Oct 3, 2023

This looks great thanks @quux00 ! Could you add an entry to the lucene/CHANGES.txt file under Lucene 9.9 please?

@javanna javanna added this to the 9.9.0 milestone Oct 3, 2023
@quux00 quux00 force-pushed the concurrent-search/wait-on-all-tasks branch from f206f52 to 6be6d13 Compare October 4, 2023 15:21
Copy link
Contributor

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@javanna javanna merged commit 7371493 into apache:main Oct 5, 2023
4 checks passed
@javanna
Copy link
Contributor

javanna commented Oct 5, 2023

Thanks @quux00 !

javanna pushed a commit that referenced this pull request Oct 5, 2023
The TaskExecutor used to run concurrent operations may leave running tasks behind when an exception is thrown by one of the tasks. This commit ensures that it instead waits for all tasks to complete before it re-throws the exception. If there's more than one exception thrown, they are going to be added as suppressed exceptions to the first one that was caught.
s1monw pushed a commit to s1monw/lucene that referenced this pull request Oct 10, 2023
…#12523)

The TaskExecutor used to run concurrent operations may leave running tasks behind when an exception is thrown by one of the tasks. This commit ensures that it instead waits for all tasks to complete before it re-throws the exception. If there's more than one exception thrown, they are going to be added as suppressed exceptions to the first one that was caught.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants