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

Use a wrapper for event loop to handle exceptions gracefully #24412

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

shangm2
Copy link
Contributor

@shangm2 shangm2 commented Jan 22, 2025

Description

  1. we saw the following error
    Screenshot 2025-01-21 at 16 26 07 and thanks to @arhimondr, the root cause is if running code on event loop and the code throws exception, the event loop thread will be killed but the event loop group will not create a new thread.

Motivation and Context

  1. Use a event loop wrapper to handle exceptions while running httpRemoteTask related jobs on event loop gracefully
  2. Also update the code so that we only add listener after the object has been fully constructed.

Impact

Test Plan

  1. Local HiveQueryRunner works great
  2. Internal verifier test passed

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Fixs
* Fix how the exceptions thrown from event loop is being handled while performing HttpRemoteTask :pr:`24412`

@shangm2 shangm2 requested a review from a team as a code owner January 22, 2025 00:31
@shangm2 shangm2 requested a review from presto-oss January 22, 2025 00:31
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Jan 22, 2025
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Please update the release notes to indicate that this is a bug fix per our release notes guidelines

@shangm2 shangm2 force-pushed the hotfix_for_eventloop branch from c9a62af to f8ee70f Compare January 22, 2025 17:15
@tdcmeehan tdcmeehan self-assigned this Jan 22, 2025
@shangm2 shangm2 force-pushed the hotfix_for_eventloop branch 2 times, most recently from eef6836 to 503a60e Compare January 23, 2025 04:46
@shangm2 shangm2 changed the title Prevent 'this' from leaking in HttpRemoteTask Use a wrapper for event loop to handle exceptions from event loop gracefully Jan 23, 2025
@shangm2 shangm2 force-pushed the hotfix_for_eventloop branch from 503a60e to a6e0424 Compare January 23, 2025 06:38
Copy link
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

Tricky bug. Didn't expect the implementation to be not exception safe. It used to be that some executors implementations in Java were not thread safe (e.g.: ScheduledExecutorService) but I remember it was fixed a while ago. Thank you for working on the fix.

@shangm2 shangm2 force-pushed the hotfix_for_eventloop branch 2 times, most recently from 8af244b to 1cf4320 Compare January 23, 2025 20:38
@shangm2 shangm2 changed the title Use a wrapper for event loop to handle exceptions from event loop gracefully Use a wrapper for event loop to handle exceptions gracefully Jan 23, 2025
@shangm2 shangm2 force-pushed the hotfix_for_eventloop branch from d758c60 to 70eae6c Compare January 23, 2025 23:48
@NikhilCollooru NikhilCollooru merged commit 48f2fb1 into prestodb:master Jan 24, 2025
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:Meta PR from Meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants