[stability] Make task result for RemoveTask optional #5146
Conversation
|
Test FAILed. |
pcmoritz
changed the title
|
Test FAILed. |
|
Test PASSed. |
|
Test FAILed. |
src/ray/raylet/scheduling_queue.cc
Outdated
| @@ -247,7 +247,8 @@ std::vector<Task> SchedulingQueue::RemoveTasks(std::unordered_set<TaskID> &task_ | |||
| return removed_tasks; | |||
| } | |||
|
|
|||
| Task SchedulingQueue::RemoveTask(const TaskID &task_id, TaskState *removed_task_state) { | |||
| boost::optional<Task> SchedulingQueue::RemoveTask(const TaskID &task_id, | |||
There was a problem hiding this comment.
What do you think about a signature like this instead?
bool SchedulingQueue::RemoveTask(const TaskID &task_id, TaskState *removed_task_state, Task *removed_task);
I was thinking that it might be good to make sure that the caller doesn't try to use the TaskState if the function returns false.
There was a problem hiding this comment.
I did that first (and it's nicer), but it involved needing a default constructor for Task and TaskSpecification and TaskExecutionSpecification :)
| @@ -1035,6 +1039,8 @@ void NodeManager::ProcessDisconnectClientMessage( | |||
| << "job_id: " << worker->GetAssignedJobId(); | |||
| } | |||
|
|
|||
| client->Close(); | |||
There was a problem hiding this comment.
Was this part of the bug?
There was a problem hiding this comment.
This was another bug where we were running out of file descriptors.
|
I updated the PR description with the current theory of which calls it affects. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test PASSed. |
|
@stephanie-wang This is ready to merge |
What do these changes do?
This fixes the long-running stress test https://github.com/ray-project/ray/blob/master/ci/long_running_tests/workloads/many_drivers.py. It fixes a race condition where Task information is removed when a driver is disconnected but then still further used.
The current hypothesis is that this concerns only the SWAP queue, so there are still some checks left in. If other queues are also affected, we might need to make them non-fatal too.
Related issue number
Linter
scripts/format.shto lint the changes in this PR.