[SPARK-31847][CORE][TESTS] DAGSchedulerSuite: Rewrite the test framework to support apply specified spark configurations. #29228

beliefer · 2020-07-25T03:08:12Z

What changes were proposed in this pull request?

DAGSchedulerSuite exists some issue:
afterEach and init are called when the SparkConf of the default SparkContext has no configuration that the test case must set. This causes the SparkContext initialized in beforeEach to be discarded without being used, resulting in waste. On the other hand, the flexibility to add configurations to SparkConf should be addressed by the test framework.

Test suites inherits LocalSparkContext can be simplified.

Why are the changes needed?

Reduce overhead about init SparkContext.
Rewrite the test framework to support apply specified spark configurations.

Does this PR introduce any user-facing change?

'No'.

How was this patch tested?

Jenkins test.

beliefer · 2020-07-25T03:08:32Z

cc @jiangxb1987 @Ngone51

SparkQA · 2020-07-25T03:17:35Z

Test build #126523 has finished for PR 29228 at commit 14de6c2.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait SparkConfHelper

SparkQA · 2020-07-25T06:05:22Z

Test build #126524 has finished for PR 29228 at commit acb4e80.

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

HyukjinKwon · 2020-07-27T03:27:09Z

core/src/test/scala/org/apache/spark/SparkConfHelper.scala

+trait SparkConfHelper {
+
+  /** Sets all configurations specified in `pairs`, calls `init`, and then calls `testFun` */
+  protected def withSparkConf(pairs: (String, String)*)(testFun: SparkConf => Any): Unit = {


SparkConfHelper.withSparkConf seems only used DAGSchedulerSuite. Let's don't make a trait for now but add it into DAGSchedulerSuite.

SparkConfHelper.withSparkConf will be used in other suite like TaskSchedulerImplSuite、TaskSetManagerSuite.

jiangxb1987 · 2020-07-30T05:56:02Z

It would be really great if you can list the test cases/suites that could get simplified by this change, thanks!

beliefer · 2020-07-30T07:37:30Z

It would be really great if you can list the test cases/suites that could get simplified by this change, thanks!

I added them to the description of this PR.

jiangxb1987 · 2020-08-09T05:35:37Z

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

+    testWithSparkConf(testName, testTags: _*)()(testFun)(pos)
+  }
+
+  private def testWithSparkConf(testName: String, testTags: Tag*)


If I understand correctly, you are doing the hack here because you need to modify the SparkConf within beforeEach (between super.beforeEach() and init()). In other words, you don't need to call testWithSparkConf instead of test, if you don't do extra initialization at the beforeEach() stage. Thus, this change is necessarily useful for those test suites you listed, right?

A more staightforward idea would likely to be having a withConfig(pairs: (String, String)*) method to create a new SparkConf with the specified configuration values？ The idea doesn't simplify DAGSchedulerSuite that much, because you still need to first stop the SparkContext then call init() with your own SparkConf, but at least it's not worse than the current approach, and more easy to understand and to be reused.

For your first question, yes, it is.
For your second question, withConfig(pairs: (String, String)*) mean to stop the SparkContext and then call init(). So I created the function testWithSparkConf which avoid to stop the SparkContext then call init().

I tend to agree with @jiangxb1987 . There're only 7 places using the testWithSparkConf() comparing to other 81 tests within the DAGSchedulerSuite. ~~We could just call init inside each test to ease other tests who needs to call afterEach inside the test.~~(Sorry, just realized that it's actually the same with current implementation)

And I have another idea for the whole thing. That is, we could probably initialize the SparkContext like this:

trait LocalSparkContext ... { @transient private var _sc: SparkContext = _ private val conf = new SparkConf() def sc: SparkContext = { if (_sc == null) { _sc = new SparkContext(conf) } _sc } def withConf(pairs: (String, String)*) = { if (_sc != null) { // probably, log warning when SparkContext already initialized // since configurations won't take effect in this case } paris.foreach { case (k, v) => conf.set(k, v) } } override def afterEach(): Unit = { try { resetSparkContext() } finally { super.afterEach() } } def resetSparkContext(): Unit = { LocalSparkContext.stop(sc) ResourceProfile.clearDefaultProfile() sc = null } } class DAGSchedulerSuite extends LocalSparkContext ... { private var firstInit: Boolean = _ override def beforeEach: Unit = { super.beforeEach() firstInit = true } override def sc: SparkContext = { val sc = super.sc() if (firstInit) { init(sc) firstInit = false } sc } }

@beliefer @jiangxb1987 WDYT?

+1 to the above proposal

OK. I will implement this proposal.

SparkQA · 2020-08-19T10:51:17Z

Test build #127645 has finished for PR 29228 at commit a1fb471.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DAGSchedulerSuite extends SparkFunSuite with LocalSparkContext with TimeLimits

SparkQA · 2020-08-19T11:12:27Z

Test build #127646 has finished for PR 29228 at commit c95f004.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-08-20T02:05:37Z

Test build #127669 has finished for PR 29228 at commit b258649.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-08-20T06:02:56Z

Test build #127672 has finished for PR 29228 at commit 69790ad.

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

Ngone51 · 2020-08-20T07:57:53Z

core/src/test/scala/org/apache/spark/LocalSparkContext.scala


 /** Manages a local `sc` `SparkContext` variable, correctly stopping it after each test. */
-trait LocalSparkContext extends BeforeAndAfterEach with BeforeAndAfterAll { self: Suite =>
+trait LocalSparkContext extends Logging


I think we usually put Logging at the end.

Ngone51 · 2020-08-20T08:01:50Z

core/src/test/scala/org/apache/spark/LocalSparkContext.scala

+      logWarning("Because SparkContext already initialized, " +
+        "since configurations won't take effect in this case.")


nit: These configurations ${paris.mkString(", ")} won't take effect since the SparkContext has been already initialized.

Ngone51 · 2020-08-20T08:04:32Z

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

    super.beforeEach()
-    init(new SparkConf())
+    firstInit = true
+    setConf("spark.master" -> "local[2]", "spark.app.name" -> "DAGSchedulerSuite")


We'd better to expose the conf via a function to set the conf. setConf is designed to be used by the test only.

Ngone51 · 2020-08-20T08:06:17Z

core/src/test/scala/org/apache/spark/LocalSparkContext.scala

+trait LocalSparkContext extends Logging
+  with BeforeAndAfterEach with BeforeAndAfterAll { self: Suite =>
+
+  private var _conf: SparkConf = new SparkConf()


I think SparkConf should have the default values for master and appName. So test suites extend it could use the SparkContext directly without any specific configurations when the test doesn't really care.

SparkQA · 2020-08-20T09:45:28Z

Test build #127686 has finished for PR 29228 at commit bd9b85f.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

beliefer · 2020-08-21T07:26:31Z

retest this please

SparkQA · 2020-08-21T10:42:24Z

Test build #127732 has finished for PR 29228 at commit c7e5e7b.

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

Ngone51 · 2020-08-21T12:12:31Z

core/src/test/scala/org/apache/spark/LocalSC.scala

+    _sc
+  }
+
+  def withConf(pairs: (String, String)*): Unit = {


Maybe like this?

def withConf(pairs: (String, String)*)(f: => Unit) = { try f finally { // reset conf here } }

If so, we don't need to create the new SparkConf for each test.

SparkQA · 2020-08-24T06:08:30Z

Test build #127827 has finished for PR 29228 at commit 86dc8f8.

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

beliefer · 2020-08-24T06:40:23Z

retest this please

SparkQA · 2020-08-24T07:05:02Z

Test build #127831 has finished for PR 29228 at commit 86dc8f8.

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

beliefer · 2020-08-24T07:48:56Z

retest this please

SparkQA · 2020-08-24T10:32:29Z

Test build #127837 has finished for PR 29228 at commit 86dc8f8.

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

Ngone51 · 2020-08-25T12:54:57Z

LGTM. BTW, could you update the PR description? You may not need to list all the test suites with the latest change but just say test suites inherits LocalSparkContext can be simplified.

@jiangxb1987 Could you also take a look?

beliefer · 2020-08-26T02:11:15Z

@Ngone51 I updated the description of this PR.

jiangxb1987 · 2020-08-29T05:00:51Z

core/src/test/scala/org/apache/spark/LocalSC.scala

+ * After migrating all test suites that use [[LocalSparkContext]] to use [[LocalSC]], we will
+ * delete the original [[LocalSparkContext]] and rename [[LocalSC]] to [[LocalSparkContext]].
+ */
+trait LocalSC extends BeforeAndAfterEach


Since this class is only used for temporary purpose, can we name it as TempLocalSparkContext ? TBH I don't like the SC name which is very vague to me.

jiangxb1987 · 2020-08-29T05:01:07Z

LGTM otherwise

SparkQA · 2020-08-29T18:01:19Z

Test build #128018 has finished for PR 29228 at commit 1029d26.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with TimeLimits

jiangxb1987

LGTM

jiangxb1987 · 2020-09-02T06:50:11Z

retest this please

SparkQA · 2020-09-02T07:05:03Z

Test build #128190 has finished for PR 29228 at commit 1029d26.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with TimeLimits

beliefer · 2020-09-03T05:55:37Z

retest this please

SparkQA · 2020-09-03T05:59:16Z

Test build #128231 has finished for PR 29228 at commit 1029d26.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with TimeLimits

beliefer · 2020-09-03T06:24:14Z

retest this please

SparkQA · 2020-09-03T06:32:38Z

Test build #128234 has finished for PR 29228 at commit 1029d26.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with TimeLimits

beliefer · 2020-09-03T08:11:42Z

retest this please

SparkQA · 2020-09-03T11:10:54Z

Test build #128244 has finished for PR 29228 at commit 1029d26.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with TimeLimits

beliefer · 2020-09-09T03:05:17Z

cc @jiangxb1987

jiangxb1987 · 2020-09-09T21:30:40Z

retest this please

SparkQA · 2020-09-10T00:23:36Z

Test build #128470 has finished for PR 29228 at commit 1029d26.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with TimeLimits

HyukjinKwon · 2020-09-14T02:57:09Z

Merged to master.

beliefer and others added 8 commits June 19, 2020 10:36

Reuse completeNextStageWithFetchFailure

4a6f903

Merge remote-tracking branch 'upstream/master'

96456e2

Merge remote-tracking branch 'upstream/master'

4314005

Merge remote-tracking branch 'upstream/master'

d6af4a7

Merge remote-tracking branch 'upstream/master'

f69094f

Merge remote-tracking branch 'upstream/master'

b86a42d

Merge branch 'master' of github.com:beliefer/spark

2ac5159

extend test framework

14de6c2

probot-autolabeler bot added the CORE label Jul 25, 2020

extend test framework

acb4e80

HyukjinKwon reviewed Jul 27, 2020

View reviewed changes

dongjoon-hyun added the TESTS label Aug 9, 2020

jiangxb1987 reviewed Aug 9, 2020

View reviewed changes

Change the implement.

a1fb471

Fix scala style.

c95f004

Fix inherits conflicting.

b258649

Fix build error.

69790ad

Ngone51 reviewed Aug 20, 2020

View reviewed changes

Optimize code.

bd9b85f

Fix conflict.

4001780

Ngone51 reviewed Aug 21, 2020

View reviewed changes

Optimize code

86dc8f8

jiangxb1987 reviewed Aug 29, 2020

View reviewed changes

Optimize code

1029d26

jiangxb1987 approved these changes Sep 2, 2020

View reviewed changes

HyukjinKwon approved these changes Sep 14, 2020

View reviewed changes

HyukjinKwon closed this in e558b8a Sep 14, 2020

		logWarning("Because SparkContext already initialized, " +
		"since configurations won't take effect in this case.")

[SPARK-31847][CORE][TESTS] DAGSchedulerSuite: Rewrite the test framework to support apply specified spark configurations. #29228

[SPARK-31847][CORE][TESTS] DAGSchedulerSuite: Rewrite the test framework to support apply specified spark configurations. #29228

Uh oh!

Conversation

beliefer commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

beliefer commented Jul 25, 2020

Uh oh!

SparkQA commented Jul 25, 2020

Uh oh!

SparkQA commented Jul 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented Jul 30, 2020

Uh oh!

beliefer commented Jul 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ngone51 Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 19, 2020

Uh oh!

SparkQA commented Aug 19, 2020

Uh oh!

SparkQA commented Aug 20, 2020

Uh oh!

SparkQA commented Aug 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 20, 2020

Uh oh!

beliefer commented Aug 21, 2020

Uh oh!

SparkQA commented Aug 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 24, 2020

Uh oh!

beliefer commented Aug 24, 2020

Uh oh!

SparkQA commented Aug 24, 2020

Uh oh!

beliefer commented Aug 24, 2020

Uh oh!

SparkQA commented Aug 24, 2020

Uh oh!

Ngone51 commented Aug 25, 2020

beliefer commented Jul 25, 2020 •

edited

Loading

Ngone51 Aug 17, 2020 •

edited

Loading