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
[SPARK-32875][CORE][TEST] TaskSchedulerImplSuite: For the pattern of submitTasks + resourceOffers + assert, extract the general method. #29754
Conversation
Test build #128694 has finished for PR 29754 at commit
|
retest this please |
Test build #128705 has finished for PR 29754 at commit
|
retest this please |
Test build #128759 has finished for PR 29754 at commit
|
private def submitTasksAndCheck( | ||
offers: IndexedSeq[WorkerOffer], | ||
taskSets: Seq[TaskSet] = Seq(FakeTask.createTaskSet(1)), | ||
taskSchedulerOpt: Option[TaskSchedulerImpl] = None) (f: Seq[TaskDescription] => Any): Any = { |
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.
We can use the global taskScheduler
as the default value?
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.
OK
@@ -140,6 +140,16 @@ class TaskSchedulerImplSuite extends SparkFunSuite with LocalSparkContext with B | |||
taskScheduler | |||
} | |||
|
|||
private def submitTasksAndCheck( | |||
offers: IndexedSeq[WorkerOffer], |
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 found that many tests use the same WorkerOffer, e.g., WorkerOffer("executor0", "host0", 1)
. Shall we make it as the default value as well?
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.
OK
assert(taskDescriptions.map(_.index).sorted == Seq(0, 1)) | ||
assert(manager.copiesRunning.take(2) === Array(1, 1)) | ||
submitTasksAndCheck(IndexedSeq(WorkerOffer("executor2", "host2", 1), | ||
WorkerOffer("executor3", "host3", 1)), Seq.empty) { taskDescriptions => |
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.
Personally, I feel it's weird that we're submitting empty task sets here... submitTasksAndCheck
maybe not appropriate for this case? Or you could redesign the submitTasksAndCheck
?
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.
OK. Let's not change this matter.
Test build #128971 has finished for PR 29754 at commit
|
Test build #131975 has finished for PR 29754 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #132112 has finished for PR 29754 at commit
|
cc @jiangxb1987 |
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
cc @jiangxb1987 |
|
Kubernetes integration test starting |
Kubernetes integration test status failure |
Kubernetes integration test starting |
Test build #136373 has finished for PR 29754 at commit
|
Kubernetes integration test status success |
Test build #136378 has finished for PR 29754 at commit
|
cc @jiangxb1987 |
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
What changes were proposed in this pull request?
TaskSchedulerImplSuite always check the results show below:
We can extract it as a generic method.
Why are the changes needed?
Extract a generic method.
Does this PR introduce any user-facing change?
'No'.
How was this patch tested?
Jenkins test