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

Revert event loop related changes #24548

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

shangm2
Copy link
Contributor

@shangm2 shangm2 commented Feb 13, 2025

Description

  1. we see some issues at certain clusters where workers are not receiving any splits. Reverting the following two prs back to unblock others so I can take time to root cause it.
    a. Use a wrapper for event loop to handle exceptions gracefully #24412
    b. Assign event loop to tasks and run a task's methods in the same loop #24288

Motivation and Context

  1. reverting event loop related changes to unblock others.

Impact

Test Plan

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 ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ... 

If release note is NOT required, use:

== NO RELEASE NOTE ==

@shangm2 shangm2 requested a review from a team as a code owner February 13, 2025 01:13
@shangm2 shangm2 requested a review from presto-oss February 13, 2025 01:13
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Feb 13, 2025
swapsmagic
swapsmagic previously approved these changes Feb 13, 2025
jainxrohit
jainxrohit previously approved these changes Feb 13, 2025
Copy link
Contributor

@jainxrohit jainxrohit left a comment

Choose a reason for hiding this comment

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

Thank you Shang!

rschlussel
rschlussel previously approved these changes Feb 13, 2025
@rschlussel
Copy link
Contributor

thanks @shangm2 can you fix the merge conflict?

FYI @ZacBlanco this change was in the 0.291 release. We're still investigating, but that release may need to be patched.

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.

We are still investigating the problem. There is a chance it could be related to differences in configuration on our side. Let's hold on with the revert for now.

rschlussel
rschlussel previously approved these changes Feb 13, 2025
@rschlussel
Copy link
Contributor

rebase onto master instead of adding a merge commit

@rschlussel rschlussel dismissed arhimondr’s stale review February 19, 2025 20:36

we'll revert this for now while we investigate and figure out how to put these changes behind a config in case of regression.

@shangm2
Copy link
Contributor Author

shangm2 commented Feb 19, 2025

rebase onto master instead of adding a merge commit

@rschlussel let me know if I should keep two revert commits so it is easy for eyes or I should squash them together. Thanks!

@rschlussel
Copy link
Contributor

I don't care either way.

@NikhilCollooru NikhilCollooru merged commit a766a3a into prestodb:master Feb 19, 2025
53 checks passed
@arhimondr
Copy link
Member

figure out how to put these changes behind a config in case of regression

We may need to create a copy of the HttpRemoteTask/TaskInfoFetcher/ContinuousTaskStatusFetcher and change the HttpRemoteTaskFactory to create current / old one based on a property.

Is it confirmed that this change is the change causing issues?

@rschlussel
Copy link
Contributor

No it's not confirmed. We still don't know the root cause.

@shangm2
Copy link
Contributor Author

shangm2 commented Feb 20, 2025

figure out how to put these changes behind a config in case of regression

We may need to create a copy of the HttpRemoteTask/TaskInfoFetcher/ContinuousTaskStatusFetcher and change the HttpRemoteTaskFactory to create current / old one based on a property.

Is it confirmed that this change is the change causing issues?

Will do.

@jainxrohit
Copy link
Contributor

figure out how to put these changes behind a config in case of regression

We may need to create a copy of the HttpRemoteTask/TaskInfoFetcher/ContinuousTaskStatusFetcher and change the HttpRemoteTaskFactory to create current / old one based on a property.

Is it confirmed that this change is the change causing issues?

This is a good suggestion!

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.

7 participants