Skip to content

[SPARK-31845][CORE][TESTS] Refactor DAGSchedulerSuite by introducing completeAndCheckAnswer and using completeNextStageWithFetchFailure #28866

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.

Sign up for GitHub

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

Jump to bottom
Closed
beliefer wants to merge 4 commits into from
Closed

[SPARK-31845][CORE][TESTS] Refactor DAGSchedulerSuite by introducing completeAndCheckAnswer and using completeNextStageWithFetchFailure #28866

beliefer wants to merge 4 commits into from

Conversation

@beliefer
Copy link
Contributor

@beliefer beliefer commented Jun 19, 2020 •
edited by dongjoon-hyun
Loading

What changes were proposed in this pull request?

First
DAGSchedulerSuite provides completeNextStageWithFetchFailure to make all tasks in non first stage occur FetchFailed.
But many test case uses complete directly as follows:

 complete(taskSets(1), Seq(
     (FetchFailed(makeBlockManagerId("hostA"),
        shuffleDep1.shuffleId, 0L, 0, 0, "ignored"), null)))

We need to reuse completeNextStageWithFetchFailure.

Second
DAGSchedulerSuite also check the results show below:

complete(taskSets(0), Seq((Success, 42)))
assert(results === Map(0 -> 42))

We can extract it as a generic method of checkAnswer.

Why are the changes needed?

Reuse completeNextStageWithFetchFailure

Does this PR introduce any user-facing change?

'No'.

How was this patch tested?

Jenkins test

@SparkQA
Copy link

SparkQA commented Jun 19, 2020

Test build #124251 has finished for PR 28866 at commit 4a6f903.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 19, 2020

Test build #124253 has finished for PR 28866 at commit 7e18dcc.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 19, 2020

Test build #124255 has finished for PR 28866 at commit fe42fd5.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Ngone51
Ngone51 reviewed Jun 19, 2020
View reviewed changes
Copy link
Member

@Ngone51 Ngone51 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems like checkAnswer only has one line change for the developer. But overall, I think it's good to have it.

core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala Outdated
private def checkAnswer(
taskSet: TaskSet,
results: Seq[(TaskEndReason, Any)],
expected: Map[Int, Any]): Unit = {
Copy link
Member

@Ngone51 Ngone51 Jun 19, 2020

Choose a reason for hiding this comment

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

nit: 4 indents

Copy link
Contributor Author

@beliefer beliefer Jun 22, 2020

Choose a reason for hiding this comment

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

OK

core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala Outdated
runEvent(JobCancelled(jobId, None))
}

/** Make task set success and check result. */
Copy link
Member

@Ngone51 Ngone51 Jun 19, 2020

Choose a reason for hiding this comment

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

It doesn't make the whole taskset success but only some tasks in the taskset success?

Copy link
Contributor Author

@beliefer beliefer Jun 22, 2020

Choose a reason for hiding this comment

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

You said right, I will change the comments.

core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala Outdated
/** Make task set success and check result. */
private def checkAnswer(
taskSet: TaskSet,
results: Seq[(TaskEndReason, Any)],
Copy link
Member

@Ngone51 Ngone51 Jun 19, 2020

Choose a reason for hiding this comment

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

I think the name of results is not appropriate here considering its usage and also confused with this.results below. Maybe, taskEndInfos?

Copy link
Contributor Author

@beliefer beliefer Jun 22, 2020

Choose a reason for hiding this comment

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

Let's update results to taskEndInfos for complete too.

core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala Outdated
}

/** Make task set success and check result. */
private def checkAnswer(
Copy link
Member

@Ngone51 Ngone51 Jun 19, 2020

Choose a reason for hiding this comment

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

Maybe, completeAndCheckAnswer?

Copy link
Contributor Author

@beliefer beliefer Jun 22, 2020

Choose a reason for hiding this comment

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

OK

@SparkQA
Copy link

SparkQA commented Jun 22, 2020

Test build #124347 has finished for PR 28866 at commit d1742fe.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@beliefer
Copy link
Contributor Author

beliefer commented Jun 22, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jun 22, 2020

Test build #124360 has finished for PR 28866 at commit d1742fe.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@Ngone51
Copy link
Member

Ngone51 commented Jun 23, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124373 has finished for PR 28866 at commit d1742fe.

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

@Ngone51
Copy link
Member

Ngone51 commented Jun 23, 2020

LGTM, also cc @jiangxb1987

@beliefer beliefer changed the title [SPARK-31845][CORE][TESTS] DAGSchedulerSuite: Reuse completeNextStageWithFetchFailure [SPARK-31845][SPARK-31843][CORE][TESTS] DAGSchedulerSuite: Reuse completeNextStageWithFetchFailure and DAGSchedulerSuite: For the pattern of complete + assert, extract the general method Jun 24, 2020
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 27, 2020

Retest this please.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-31845][SPARK-31843][CORE][TESTS] DAGSchedulerSuite: Reuse completeNextStageWithFetchFailure and DAGSchedulerSuite: For the pattern of complete + assert, extract the general method [SPARK-31845][CORE][TESTS] Refactor DAGSchedulerSuite by introducing completeAndCheckAnswer Jun 27, 2020
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-31845][CORE][TESTS] Refactor DAGSchedulerSuite by introducing completeAndCheckAnswer [SPARK-31845][CORE][TESTS] Refactor DAGSchedulerSuite by introducing completeAndCheckAnswer and using completeNextStageWithFetchFailure Jun 27, 2020
dongjoon-hyun
dongjoon-hyun approved these changes Jun 27, 2020
View reviewed changes
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. Thank you, @beliefer and @Ngone51 .
Merged to master.

@SparkQA
Copy link

SparkQA commented Jun 27, 2020

Test build #124556 has finished for PR 28866 at commit d1742fe.

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

@beliefer
Copy link
Contributor Author

beliefer commented Jun 28, 2020

@dongjoon-hyun @Ngone51 Thanks for your help!

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

Reviewers

@Ngone51 Ngone51 Ngone51 left review comments

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

CORE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@beliefer @SparkQA @Ngone51 @dongjoon-hyun