Skip to content

Conversation

@weixiuli
Copy link
Contributor

What changes were proposed in this pull request?

Avoid to recompute the pending speculative tasks in the ExecutorAllocationManager, and remove some unnecessary code.

Why are the changes needed?

The number of the pending speculative tasks is recomputed in the ExecutorAllocationManager to calculate the maximum number of executors required. While , it only needs to be computed once to improve performance.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests.

…in the ExecutorAllocationManager and remove some unnecessary code.
@github-actions github-actions bot added the CORE label Apr 23, 2021
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@weixiuli
Copy link
Contributor Author

@cloud-fan @Ngone51 Kindly review, thanks.

val unschedulableTaskSets = listener.pendingUnschedulableTaskSetsPerResourceProfile(rpId)
val running = listener.totalRunningTasksPerResourceProfile(rpId)
val numRunningOrPendingTasks = pending + running
val numRunningOrPendingTasks = pendingTask + pendingSpeculative + running
Copy link
Contributor

Choose a reason for hiding this comment

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

previously pending is pendingTasksPerResourceProfile(rp) + pendingSpeculativeTasksPerResourceProfile(rp), now we use pendingTasksPerResourceProfile() + pendingSpeculative, and pendingSpeculative simply calls pendingSpeculativeTasksPerResourceProfile(...)

seems like a straightforward code clean up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously the pendingSpeculativeTasksPerResourceProfile(rp) will be called not only by totalPendingTasksPerResourceProfile(rpId) but also by val pendingSpeculative = listener.pendingSpeculativeTasksPerResourceProfile(rpId), this PR we only call the pendingSpeculativeTasksPerResourceProfile(rp) once to get the pendingSpeculativeTasks , and use it for numRunningOrPendingTasks and pendingSpeculative.

Copy link
Member

@Ngone51 Ngone51 left a comment

Choose a reason for hiding this comment

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

LGTM

@Ngone51
Copy link
Member

Ngone51 commented Apr 23, 2021

cc @tgravescs

@dongjoon-hyun
Copy link
Member

Thank you, @weixiuli and all.
Merged to master for Apache Spark 3.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants