-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 stuck issue for the load testing of Push-based task scheduling #2006
Conversation
…ServerEvent::JobSubmitted in offer_resources()
@@ -70,12 +70,28 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan> | |||
return Ok(Some(SchedulerServerEvent::JobSubmitted(job_id))); | |||
} | |||
|
|||
let mut executors_delta_data: Vec<ExecutorDeltaData> = available_executors |
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.
executors_delta_data
is confused for me, because this just clone/copy some attributes from available_executors
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.
ResourceChange
or SlotsChange
probably?
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.
fixed
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.
LTGM
except for some naming
Thanks @yahoNanJing @liukun4515 |
Which issue does this PR close?
Closes #2005 .
Rationale for this change
What changes are included in this PR?
For the first bug:
We should make sure the returned available_executors should be all with available_task_slots > 0.
For the second bug:
We should use update_executor_data rather than save_executor_data to update executors' available_task_slots.
Are there any user-facing changes?