-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Fix pull manager deadlock due to object reconstruction #24791
Conversation
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.
I like the high-level change! One thing I'm not sure about is if it will matter that we're no longer preserving order between requests as they go between active and inactive. To be safe, I think we should use map instead of set to store active/inactive requests. One scenario where it could matter, for example, is if we have a large request that has been active for a long time, and we end up deactivating that over another later request.
^ Ignore that, I forgot std::set is ordered :) |
@@ -141,19 +139,16 @@ class PullManager { | |||
void ResetRetryTimer(const ObjectID &object_id); | |||
|
|||
/// The number of ongoing object pulls. | |||
int NumActiveRequests() const; | |||
int NumObjectPullRequests() const; |
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.
Was this just a badly named method before?
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.
Yes, just a bad naming. We are counting object_pull_reqeusts_
not active_object_pull_requests_
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.
Nice, this is great! Let's also check if it's working on the chaos test in parallel?
Release tests look good: https://buildkite.com/ray-project/release-tests-pr/builds/3441 |
Why are these changes needed?
When an object is under reconstruction, pull manager keeps the bundle request active with no timeout, which may block the next bundle request that's needed for the object reconstruction. As a result, we have deadlock.
For example, task 1 takes object A as argument and returns object B, task 2 takes object B as argument. When we run task 2, pull manager will add B to the queue and then B is lost. In this case, task 1 is re-submitted and A is added the the pull manager queue after B (assuming both tasks are scheduled to the same node). Due to limited available object store memory, A cannot be activated until B is pulled but B cannot be pulled until A is pulled and B is reconstructed.
The solution is that if an active pull request has pending-creation objects, pull manager will deactivates it until creation is done. This way, we will free object store memory occupied by the current active pull request so that next requests can proceed and potentially unblock the object creation.
Related issue number
Closes #13689
Checks
scripts/format.sh
to lint the changes in this PR.