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

Allow stop to interrupt the consumer thread and safely release the resource #13418

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Jun 17, 2024

Solve #13398 and #10861, and maybe #13263

  • Make stop() interrupt the consumer thread if it is not invoked by the same thread, and wait for it to join
  • Ignore the exception handling in consumer thread if stop() is already invoked (_shouldStop is set)
  • With the above mechanisms, when stop() is invoked, we can guarantee the consumer thread is stopped, and we may safely release the resources held by the consumer

@Jackie-Jiang
Copy link
Contributor Author

@sajjad-moradi @mcvsubbu I don't find a good way to add a unit test for it. Can you please help verify the fix?

@mcvsubbu
Copy link
Contributor

Sajjad and I will review this and get back to you. Please hold merging

Copy link
Contributor

@sajjad-moradi sajjad-moradi left a comment

Choose a reason for hiding this comment

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

The fix looks good to me.
Also I simulated the SEGV scenario by modifying an integration test, and verified that the fix actually does the intended behavior.

@sajjad-moradi
Copy link
Contributor

sajjad-moradi commented Jun 19, 2024

Subbu also wants to take a 2nd look. Could you please hold off merging till we have his review as well?

Copy link
Contributor

@mcvsubbu mcvsubbu left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@Jackie-Jiang Jackie-Jiang merged commit 093827b into apache:master Jun 20, 2024
20 checks passed
@Jackie-Jiang Jackie-Jiang deleted the fix_segment_commit branch June 20, 2024 16:22
suyashpatel98 pushed a commit to suyashpatel98/pinot that referenced this pull request Jul 6, 2024
yashmayya pushed a commit to yashmayya/pinot that referenced this pull request Jul 24, 2024
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.

4 participants