-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
b059897
to
15db365
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Shang!
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. |
There was a problem hiding this 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.
6ed220a
15db365
to
6ed220a
Compare
rebase onto master instead of adding a merge commit |
we'll revert this for now while we investigate and figure out how to put these changes behind a config in case of regression.
7933756
to
9612ad5
Compare
@rschlussel let me know if I should keep two revert commits so it is easy for eyes or I should squash them together. Thanks! |
I don't care either way. |
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? |
No it's not confirmed. We still don't know the root cause. |
Will do. |
This is a good suggestion! |
Description
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
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: