[SPARK-34273][CORE] Do not reregister BlockManager when SparkContext is stopped #31373

dongjoon-hyun · 2021-01-28T03:43:03Z

What changes were proposed in this pull request?

This PR aims to prevent HeartbeatReceiver asks Executor to re-register blocker manager when the SparkContext is already stopped.

Why are the changes needed?

Currently, HeartbeatReceiver blindly asks re-registration for the new heartbeat message.
However, when SparkContext is stopped, we don't need to re-register new block manager.
Re-registration causes unnecessary executors' logs and and a delay on job termination.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the CIs with the newly added test case.

…is stopped

dongjoon-hyun · 2021-01-28T06:33:33Z

Retest this please

SparkQA · 2021-01-28T07:34:13Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39184/

SparkQA · 2021-01-28T08:05:10Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39184/

SparkQA · 2021-01-28T08:53:10Z

Test build #134594 has finished for PR 31373 at commit 510a505.

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

dongjoon-hyun · 2021-01-28T09:09:28Z

Retest this please

Ngone51 · 2021-01-28T09:45:09Z

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

@@ -128,14 +128,16 @@ private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)

    // Messages received from executors
    case heartbeat @ Heartbeat(executorId, accumUpdates, blockManagerId, executorUpdates) =>
+      var reregisterBlockManager = !sc.isStopped


This sounds like a race condition that happens during SparkContext shutdown. So it's also possible that the SparkContext is stopped right after we sent the HeartbeatResponse. In that case, IIUC, the issue will still exist.

Does the current behavior cause any real issue?

Thank. you for review, @Ngone51 . That's true. In the production environment, I hit those intermediate status. And, this will help us simplify the situation.

The case you mentioned, Send HeartbeatResponse and sc.stop invoked, is a normal situation. The users don't complain about this.

The case in this PR, sc.stop invoked and Spark works inefficiently by sending HeartbeatResponse(true) is a problem. The users complain about this.

For the following, yes. We are After the apps sc.stop takes a longer time than we expect.

Does the current behavior cause any real issue?

SparkQA · 2021-01-28T10:41:50Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39199/

AmplabJenkins · 2021-01-28T10:45:46Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39199/

SparkQA · 2021-01-28T12:10:58Z

Test build #134611 has finished for PR 31373 at commit 510a505.

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

AmplabJenkins · 2021-01-28T12:33:44Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134611/

dongjoon-hyun · 2021-01-28T17:52:42Z

Also, cc @mridulm and @tgravescs

tgravescs · 2021-01-28T20:51:55Z

@dongjoon-hyun can you update description with what user sees from this? I think its a bunch of extra log messages and shutdown takes longer?

dongjoon-hyun · 2021-01-28T20:53:36Z

Thank you for review, @tgravescs . Yes, it dose. I'll update it.

dongjoon-hyun · 2021-01-28T20:55:42Z

Updated!

tgravescs

overall changes seem fine to me. Spark has bunch of cases where shutdown can be slow and spews a bunch of log messages, so getting rid of those is great even if its not perfect.

dongjoon-hyun · 2021-01-28T21:05:48Z

Thank you so much, @tgravescs !

…is stopped ### What changes were proposed in this pull request? This PR aims to prevent `HeartbeatReceiver` asks `Executor` to re-register blocker manager when the SparkContext is already stopped. ### Why are the changes needed? Currently, `HeartbeatReceiver` blindly asks re-registration for the new heartbeat message. However, when SparkContext is stopped, we don't need to re-register new block manager. Re-registration causes unnecessary executors' logs and and a delay on job termination. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs with the newly added test case. Closes #31373 from dongjoon-hyun/SPARK-34273. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit bc41c5a) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…is stopped This PR aims to prevent `HeartbeatReceiver` asks `Executor` to re-register blocker manager when the SparkContext is already stopped. Currently, `HeartbeatReceiver` blindly asks re-registration for the new heartbeat message. However, when SparkContext is stopped, we don't need to re-register new block manager. Re-registration causes unnecessary executors' logs and and a delay on job termination. No. Pass the CIs with the newly added test case. Closes #31373 from dongjoon-hyun/SPARK-34273. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit bc41c5a) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2021-01-28T21:16:43Z

Merged to master/3.1/3.0/2.4.

…is stopped ### What changes were proposed in this pull request? This PR aims to prevent `HeartbeatReceiver` asks `Executor` to re-register blocker manager when the SparkContext is already stopped. ### Why are the changes needed? Currently, `HeartbeatReceiver` blindly asks re-registration for the new heartbeat message. However, when SparkContext is stopped, we don't need to re-register new block manager. Re-registration causes unnecessary executors' logs and and a delay on job termination. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs with the newly added test case. Closes apache#31373 from dongjoon-hyun/SPARK-34273. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

github-actions bot added the CORE label Jan 28, 2021

This comment has been minimized.

Sign in to view

dongjoon-hyun marked this pull request as draft January 28, 2021 05:41

[SPARK-34273][CORE] Do not reregister BlockManager when SparkContext …

510a505

…is stopped

This comment has been minimized.

Sign in to view

dongjoon-hyun marked this pull request as ready for review January 28, 2021 06:33

Ngone51 reviewed Jan 28, 2021

View reviewed changes

tgravescs approved these changes Jan 28, 2021

View reviewed changes

dongjoon-hyun closed this in bc41c5a Jan 28, 2021

dongjoon-hyun deleted the SPARK-34273 branch January 28, 2021 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34273][CORE] Do not reregister BlockManager when SparkContext is stopped #31373

[SPARK-34273][CORE] Do not reregister BlockManager when SparkContext is stopped #31373

dongjoon-hyun commented Jan 28, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dongjoon-hyun commented Jan 28, 2021

SparkQA commented Jan 28, 2021

SparkQA commented Jan 28, 2021

SparkQA commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

Ngone51 Jan 28, 2021

dongjoon-hyun Jan 28, 2021 •

edited

Loading

SparkQA commented Jan 28, 2021

AmplabJenkins commented Jan 28, 2021

SparkQA commented Jan 28, 2021

AmplabJenkins commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

tgravescs commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

tgravescs left a comment

dongjoon-hyun commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

[SPARK-34273][CORE] Do not reregister BlockManager when SparkContext is stopped #31373

[SPARK-34273][CORE] Do not reregister BlockManager when SparkContext is stopped #31373

Conversation

dongjoon-hyun commented Jan 28, 2021 • edited Loading

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?

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dongjoon-hyun commented Jan 28, 2021

SparkQA commented Jan 28, 2021

SparkQA commented Jan 28, 2021

SparkQA commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

Ngone51 Jan 28, 2021

Choose a reason for hiding this comment

dongjoon-hyun Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jan 28, 2021

AmplabJenkins commented Jan 28, 2021

SparkQA commented Jan 28, 2021

AmplabJenkins commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

tgravescs commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

tgravescs left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021

dongjoon-hyun commented Jan 28, 2021 •

edited

Loading

dongjoon-hyun Jan 28, 2021 •

edited

Loading