Skip to content

Conversation

@Ngone51
Copy link
Member

@Ngone51 Ngone51 commented Sep 14, 2020

What changes were proposed in this pull request?

This PR proposes to avoid scheduling the (non-zombie) TaskSetManager which has no pending tasks.

Why are the changes needed?

Currently, Spark always tries to schedule a (non-zombie) TaskSetManager even if it has no pending tasks. This causes notable problems for the barrier TaskSetManager: 1. calculateAvailableSlots can be called for multiple times for a launched barrier TaskSetManager; 2. user would see "Skip current round of resource offers for barrier stage" log message for
a launched barrier TaskSetManager all the time until the barrier TaskSetManager finishes, which is quite confused.

Besides, scheduling a TaskSetManager always involves many function invocations even if there're no pending tasks.

Therefore, I think we can skip those un-schedulable TasksetManagers to avoid the potential overhead.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass existing tests.

@Ngone51
Copy link
Member Author

Ngone51 commented Sep 14, 2020

@tgravescs @jiangxb1987 @cloud-fan Any idea on this?

Copy link
Contributor

@tgravescs tgravescs left a comment

Choose a reason for hiding this comment

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

definitely seems like a good thing in general, I was a little worried about other side affects that happen in the scheduling loop but the only one I saw was with locality which doesn't matter since no pending tasks so I think its ok.

@SparkQA
Copy link

SparkQA commented Sep 14, 2020

Test build #128660 has finished for PR 29750 at commit 33dba15.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. It seems that @mridulm 's comment is the last one we need to do.

@Ngone51
Copy link
Member Author

Ngone51 commented Sep 15, 2020

Thanks all! I've addressed the comment.

@dongjoon-hyun
Copy link
Member

Merged to master for Apache Spark 3.1.0 on December 2020.

@dongjoon-hyun
Copy link
Member

Thank you, @Ngone51 and all!

@SparkQA
Copy link

SparkQA commented Sep 15, 2020

Test build #128689 has finished for PR 29750 at commit 718590d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

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.

6 participants