[SPARK-25017][Core] Add test suite for ContextBarrierState #22165

xuanyuanking · 2018-08-21T05:27:17Z

What changes were proposed in this pull request?

Currently ContextBarrierState is only covered by end-to-end test in BarrierTaskContextSuite, add BarrierCoordinatorSuite to test both classes.

How was this patch tested?

UT newly added in ContextBarrierStateSuite.

…BarrierState

xuanyuanking · 2018-08-21T06:36:30Z

cc @jiangxb1987

SparkQA · 2018-08-21T07:05:01Z

Test build #94994 has finished for PR 22165 at commit 21bd1c3.

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

xuanyuanking · 2018-08-21T09:03:44Z

retest this please.

SparkQA · 2018-08-21T13:29:27Z

Test build #95007 has finished for PR 22165 at commit 21bd1c3.

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

xuanyuanking · 2018-08-22T02:22:35Z

cc @gatorsmile @cloud-fan

gatorsmile · 2018-08-22T05:26:52Z

cc @jiangxb1987 @mengxr

gatorsmile · 2018-08-22T05:27:03Z

@xuanyuanking thanks for helping the test coverage!

jiangxb1987 · 2018-08-22T05:28:39Z

I'll make one pass of this later today :) Thanks for taking this task!

xuanyuanking · 2018-08-22T14:42:16Z

My pleasure, just find this during glance over jira in recent days. :)

jiangxb1987 · 2018-08-22T16:05:01Z

One general idea is that we don't need to rely on the RPC framework to test ContextBarrierState, just mock RpcCallContexts should be enough (haven't go into detail so correct me if I'm wrong).
We shall cover the following scenarios:

RequestToSync that carries different numTasks;
RequestToSync that carries different barrierEpoch;
Collect enough RequestToSync messages before timeout;
Don't collect enough RequestToSync messages before timeout;
Handle RequestToSync from different stage attempts concurrently;
Make sure we clear all the internal data under each case.

xuanyuanking · 2018-08-23T05:53:36Z

@jiangxb1987 Great thanks for your comment!

One general idea is that we don't need to rely on the RPC framework to test ContextBarrierState, just mock RpcCallContexts should be enough.

Actually I also want to implement like this at first also as you asked in jira, but ContextBarrierState is the private inner class in BarrierCoordinator. Could I do the refactor of moving ContextBarrierState out of BarrierCoordinator? If that is permitted I think we can just mock RpcCallContext to reach this.

We shall cover the following scenarios:

Pretty cool for the list, the 5 in front scenarios are including in currently implement, I'll add the last checking work of Make sure we clear all the internal data under each case. after we reach an agreement.

xuanyuanking · 2018-09-05T13:14:35Z

Could I do the refactor of moving ContextBarrierState out of BarrierCoordinator?

gental ping @jiangxb1987, I still follow up this. :)

jiangxb1987 · 2018-09-05T13:55:42Z

I think it should be fine to make ContextBarrierState private[spark] to test it, WDYT @mengxr ?

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

mengxr · 2018-09-05T15:48:45Z

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

+        barrierEpoch = 0),
+      timeout = new RpcTimeout(5 seconds, "rpcTimeOut"))
+    // sleep for waiting barrierEpoch value change
+    Thread.sleep(500)


Do not use explicit sleep. It basically means adding 0.5 seconds to total test time and flakyness. Use conditional wait, for example: bfb7439#diff-a90010f459c27926238d7a4ce5a0aca1R107

Thanks for guidance, done in ecf12bd. I'll also pay attention in the future work.

mengxr · 2018-09-05T15:49:33Z

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

+      }.start()
+    }
+    // sleep for waiting barrierEpoch value change
+    Thread.sleep(500)


mengxr · 2018-09-05T15:51:31Z

I didn't make a full pass over the tests. @jiangxb1987 let me know if you need me to take a pass.

xuanyuanking · 2018-09-06T13:59:48Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

    }
+
+    // Check for clearing internal data, visible for test only.
+    private[spark] def cleanCheck(): Boolean = requesters.isEmpty && timerTask == null


Add internal data clean check in Xingbo's comments: #22165 (comment).

SparkQA · 2018-09-06T18:55:41Z

Test build #95761 has finished for PR 22165 at commit ecf12bd.

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

xuanyuanking · 2018-09-17T13:32:49Z

gental ping @jiangxb1987

kiszk · 2018-09-17T14:35:27Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

    // to identify each barrier() call. It shall get increased when a barrier() call succeeds, or
    // reset when a barrier() call fails due to timeout.
-    private var barrierEpoch: Int = 0
+    private[spark] var barrierEpoch: Int = 0


nit: Is it better to add a getter method? This is because to make var visible may cause unexpected update of the variable.

Make sense, done in ec8466a。

SparkQA · 2018-09-18T11:35:42Z

Test build #96173 has finished for PR 22165 at commit ec8466a.

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

xuanyuanking · 2018-09-18T11:44:34Z

retest this please.

SparkQA · 2018-09-18T15:41:12Z

Test build #96176 has finished for PR 22165 at commit ec8466a.

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

xuanyuanking · 2018-09-19T01:33:32Z

UT fixed by #22452.

xuanyuanking · 2018-09-19T01:46:15Z

retest this please.

SparkQA · 2018-09-19T06:27:45Z

Test build #96216 has finished for PR 22165 at commit ec8466a.

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

xuanyuanking · 2018-09-26T13:52:24Z

gental ping @jiangxb1987 @kiszk

jiangxb1987 · 2018-09-26T14:35:04Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

+    private[spark] def cleanCheck(): Boolean = requesters.isEmpty && timerTask == null
+
+    // Get currently barrier epoch, visible for test only.
+    private[spark] def getBarrierEpoch(): Int = barrierEpoch


Why not just make barrierEpoch visible for testing?

#22165 (comment) As the comment here, need revert back?

jiangxb1987 · 2018-09-26T14:36:46Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

    }
+
+    // Check for clearing internal data, visible for test only.
+    private[spark] def cleanCheck(): Boolean = requesters.isEmpty && timerTask == null


nit: cleanCheck() -> isInternalStateClear()

Thanks, done in fd4d150.

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

xuanyuanking · 2018-09-27T16:02:57Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

      if (epoch != barrierEpoch) {
        requester.sendFailure(new SparkException(s"The request to sync of $barrierId with " +
-          s"barrier epoch $barrierEpoch has already finished. Maybe task $taskId is not " +
+          s"barrier epoch $epoch has already finished. Maybe task $taskId is not " +


During write the UT for ContextBarrierState, I think this is a typo in message? @jiangxb1987 Please correct me if I'm wrong.

SparkQA · 2018-09-27T20:49:39Z

Test build #96702 has finished for PR 22165 at commit 8cd78a9.

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

SparkQA · 2018-09-27T21:22:33Z

Test build #96703 has finished for PR 22165 at commit fd4d150.

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

jiangxb1987 · 2018-10-11T15:39:23Z

Actually my original thinking was like this:

      val state = new ContextBarrierState(barrierId, numTasks)
      val requester = mockRequester()
      val request = forgeRequest(numTasks, stageId, stageAttemptId, taskAttemptId, barrierEpoch)
      state.handleRequest(requester, request)
      // Verify states
      ...
      // Verify cleanup
      ...

So you don't have to launch a SparkContext for the test. Could you please check whether this is feasible?

xuanyuanking · 2018-10-13T10:13:11Z

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

+  }
+
+  test("normal test for single task") {
+    val barrierCoordinator = new BarrierCoordinator(


So you don't have to launch a SparkContext for the test. Could you please check whether this is feasible?

Thanks for Xingbo's guidance and sorry for misunderstand at first. That's feasible. But maybe this is the last thing not clear cause we still need a real BarrierCoordinator. Because a mock one will cause the timer NPE. Thanks @jiangxb1987

spark/core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

Line 152 in 2eaf058

timer.schedule(timerTask, timeoutInSecs * 1000)

SparkQA · 2018-10-13T14:25:41Z

Test build #97337 has finished for PR 22165 at commit aea2fa0.

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

xuanyuanking · 2018-11-06T12:51:35Z

retest this please.

SparkQA · 2018-11-06T16:45:47Z

Test build #98516 has finished for PR 22165 at commit aea2fa0.

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

xuanyuanking · 2018-11-07T01:53:47Z

gental ping @jiangxb1987

github-actions · 2020-01-08T00:07:29Z

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.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

[SPARK-25017][Core] Add test suite for BarrierCoordinator and Context…

21bd1c3

…BarrierState

mengxr requested changes Sep 5, 2018

View reviewed changes

Address comments and add clean check for internal data.

ecf12bd

xuanyuanking commented Sep 6, 2018

View reviewed changes

kiszk reviewed Sep 17, 2018

View reviewed changes

Address comment by kiszk

ec8466a

jiangxb1987 requested changes Sep 26, 2018

View reviewed changes

Get rid of RPC framework in UT

8cd78a9

xuanyuanking commented Sep 27, 2018

View reviewed changes

address comments

fd4d150

xuanyuanking changed the title ~~[SPARK-25017][Core] Add test suite for BarrierCoordinator and ContextBarrierState~~ [SPARK-25017][Core] Add test suite for ContextBarrierState Oct 13, 2018

Don't launch SparkContext

aea2fa0

xuanyuanking commented Oct 13, 2018

View reviewed changes

dongjoon-hyun added the SPARK CORE label Jun 14, 2019

github-actions bot added the Stale label Jan 8, 2020

github-actions bot closed this Jan 9, 2020

[SPARK-25017][Core] Add test suite for ContextBarrierState #22165

[SPARK-25017][Core] Add test suite for ContextBarrierState #22165

Uh oh!

Conversation

xuanyuanking commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

xuanyuanking commented Aug 21, 2018

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

xuanyuanking commented Aug 21, 2018

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

xuanyuanking commented Aug 22, 2018

Uh oh!

gatorsmile commented Aug 22, 2018

Uh oh!

gatorsmile commented Aug 22, 2018

Uh oh!

jiangxb1987 commented Aug 22, 2018

Uh oh!

xuanyuanking commented Aug 22, 2018

Uh oh!

jiangxb1987 commented Aug 22, 2018

Uh oh!

xuanyuanking commented Aug 23, 2018

Uh oh!

xuanyuanking commented Sep 5, 2018

Uh oh!

jiangxb1987 commented Sep 5, 2018

Uh oh!

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!

mengxr commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 6, 2018

Uh oh!

xuanyuanking commented Sep 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 18, 2018

Uh oh!

xuanyuanking commented Sep 18, 2018

Uh oh!

SparkQA commented Sep 18, 2018

Uh oh!

xuanyuanking commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuanyuanking commented Sep 19, 2018

Uh oh!

SparkQA commented Sep 19, 2018

Uh oh!

xuanyuanking commented Sep 26, 2018

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!

Uh oh!

xuanyuanking Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

xuanyuanking commented Aug 21, 2018 •

edited

Loading

xuanyuanking commented Sep 19, 2018 •

edited

Loading

xuanyuanking Sep 27, 2018 •

edited

Loading