[SPARK-23197][STREAMING][TESTS] Fix ReceiverSuite."receiver_life_cycle" to not rely on timing #25862

HeartSaVioR · 2019-09-20T01:16:57Z

What changes were proposed in this pull request?

This patch changes ReceiverSuite."receiver_life_cycle" to record actual calls with timestamp in FakeReceiver/FakeReceiverSupervisor, which doesn't rely on timing of stopping and starting receiver in restarting receiver. It enables us to give enough huge timeout on verification of restart as we can verify both stopping and starting together.

Why are the changes needed?

The test is flaky without this patch. We increased timeout to fix flakyness of this test (15adcc8) but even with longer timeout it has been still failing intermittently.

Does this PR introduce any user-facing change?

No

How was this patch tested?

I've reproduced test failure artificially via below diff:

diff --git a/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala b/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala
index faf6db82d5..d8977543c0 100644
--- a/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala
+++ b/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala
@@ -191,9 +191,11 @@ private[streaming] abstract class ReceiverSupervisor(
       // thread pool.
       logWarning("Restarting receiver with delay " + delay + " ms: " + message,
         error.getOrElse(null))
+      Thread.sleep(1000)
       stopReceiver("Restarting receiver with delay " + delay + "ms: " + message, error)
       logDebug("Sleeping for " + delay)
       Thread.sleep(delay)
+      Thread.sleep(1000)
       logInfo("Starting receiver again")
       startReceiver()
       logInfo("Receiver started again")

and confirmed this patch doesn't fail with the change.

…ot rely on timing

SparkQA · 2019-09-20T01:26:33Z

Test build #111033 has finished for PR 25862 at commit 4ff9f7e.

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

SparkQA · 2019-09-20T02:19:42Z

Test build #111035 has finished for PR 25862 at commit b4462dd.

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

SparkQA · 2019-09-20T04:10:11Z

Test build #111039 has finished for PR 25862 at commit f70b3c1.

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

dongjoon-hyun · 2019-09-20T04:17:45Z

streaming/src/test/scala/org/apache/spark/streaming/ReceiverSuite.scala

+    executor.callsRecorder.reset()
+    receiver.callsRecorder.reset()
+    receiver.restart("restarting", null, 100)
+    eventually(timeout(10.seconds), interval(10.milliseconds)) {


So, 10.seconds is enough? Or, do you need to re-trigger this PR to validate more?
BTW, thank you so much for taking care of this case! This is really an long standing issue.

Yes that was actually 1.3 seconds (300ms + 1s) and it hasn't been failing for high probability so it should be pretty enough.

HeartSaVioR · 2019-09-20T20:10:41Z

cc. @tdas @sameeragarwal as they've authored and reviewed 15adcc8
also cc. to @zsxwing @jose-torres @gaborgsomogyi

dongjoon-hyun · 2019-09-25T17:52:14Z

streaming/src/test/scala/org/apache/spark/streaming/ReceiverSuite.scala

-      assert(executor.isReceiverStarted)
+    executor.callsRecorder.reset()
+    receiver.callsRecorder.reset()
+    receiver.restart("restarting", null, 100)


This goes down from 600 to 100?

Ah yes we no longer need so long delay as we don't rely on timing.

dongjoon-hyun

I also verified this PR with the given test method. New test suite is logically robust and doesn't fail.
+1, LGTM. Merged to master.

Thank you so much, @HeartSaVioR !

dongjoon-hyun · 2019-09-25T18:00:37Z

Could you make a backporting PR, @HeartSaVioR ?

HeartSaVioR · 2019-09-25T20:02:07Z

Thanks for reviewing and merging, @dongjoon-hyun ! I'll submit a PR for branch-2.4 as well.

…e" to not rely on timing This patch changes ReceiverSuite."receiver_life_cycle" to record actual calls with timestamp in FakeReceiver/FakeReceiverSupervisor, which doesn't rely on timing of stopping and starting receiver in restarting receiver. It enables us to give enough huge timeout on verification of restart as we can verify both stopping and starting together. The test is flaky without this patch. We increased timeout to fix flakyness of this test (apache@15adcc8) but even with longer timeout it has been still failing intermittently. No I've reproduced test failure artificially via below diff: ``` diff --git a/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala b/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala index faf6db8..d8977543c0 100644 --- a/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala +++ b/streaming/src/main/scala/org/apache/spark/streaming/receiver/ReceiverSupervisor.scala -191,9 +191,11 private[streaming] abstract class ReceiverSupervisor( // thread pool. logWarning("Restarting receiver with delay " + delay + " ms: " + message, error.getOrElse(null)) + Thread.sleep(1000) stopReceiver("Restarting receiver with delay " + delay + "ms: " + message, error) logDebug("Sleeping for " + delay) Thread.sleep(delay) + Thread.sleep(1000) logInfo("Starting receiver again") startReceiver() logInfo("Receiver started again") ``` and confirmed this patch doesn't fail with the change. Closes apache#25862 from HeartSaVioR/SPARK-23197-v2. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

HeartSaVioR · 2019-09-25T20:35:30Z

#25930

dongjoon-hyun · 2019-09-25T20:46:25Z

Thanks!

[SPARK-23197][STREAMING] Fix ReceiverSuite."receiver_life_cycle" to n…

4ff9f7e

…ot rely on timing

HeartSaVioR changed the title ~~[SPARK-23197][STREAMING] Fix ReceiverSuite."receiver_life_cycle" to not rely on timing~~ [SPARK-23197][STREAMING][TESTS] Fix ReceiverSuite."receiver_life_cycle" to not rely on timing Sep 20, 2019

fix silly newline miss

b4462dd

dongjoon-hyun added the DSTREAMS label Sep 20, 2019

Fix another UT failure affected by the change

f70b3c1

dongjoon-hyun reviewed Sep 20, 2019

View reviewed changes

dongjoon-hyun added the TESTS label Sep 20, 2019

dongjoon-hyun reviewed Sep 25, 2019

View reviewed changes

dongjoon-hyun approved these changes Sep 25, 2019

View reviewed changes

dongjoon-hyun closed this in a1b90bf Sep 25, 2019

HeartSaVioR deleted the SPARK-23197-v2 branch September 25, 2019 20:35

[SPARK-23197][STREAMING][TESTS] Fix ReceiverSuite."receiver_life_cycle" to not rely on timing #25862

[SPARK-23197][STREAMING][TESTS] Fix ReceiverSuite."receiver_life_cycle" to not rely on timing #25862

Uh oh!

Conversation

HeartSaVioR commented Sep 20, 2019 • 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!

SparkQA commented Sep 20, 2019

Uh oh!

SparkQA commented Sep 20, 2019

Uh oh!

SparkQA commented Sep 20, 2019

Uh oh!

dongjoon-hyun Sep 20, 2019 • 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!

HeartSaVioR Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment • 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!

dongjoon-hyun commented Sep 25, 2019

Uh oh!

HeartSaVioR commented Sep 25, 2019

Uh oh!

HeartSaVioR commented Sep 25, 2019

Uh oh!

dongjoon-hyun commented Sep 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HeartSaVioR commented Sep 20, 2019 •

edited

Loading

dongjoon-hyun Sep 20, 2019 •

edited

Loading

HeartSaVioR commented Sep 20, 2019 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading