[SPARK-19147][CORE] Gracefully handle error in task after executor is stopped #25759

colinmjj · 2019-09-11T13:04:26Z

What changes were proposed in this pull request?

TransportClientFactory.createClient() is called by task and TransportClientFactory.close() is called by executor.
When stop the executor, close() will set workerGroup = null, NPE will occur in createClient which generate many exception in log.
For exception occurs after close(), treated it as an expected Exception
and transform it to InterruptedException which can be processed by Executor.

Why are the changes needed?

The change can reduce the exception stack trace in log file, and user won't be confused by these excepted exception.

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

New tests are added in TransportClientFactorySuite and ExecutorSuite

srowen · 2019-09-11T13:45:18Z

Wait, why would we be creating a new client after the executor is shut down?

colinmjj · 2019-09-12T02:01:27Z

Here is the scene, when the executor is killed before shuffle process, but the task was created and ready to fetch blocks. Then, the NPE will occur if task try to create a new client.
Such NPE will confuse user to find why executor is killed.

srowen · 2019-09-12T13:35:08Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

+        // and transform it to InterruptedException which can be processed by Executor.
+        // See SPARK-19147
+        if (workerGroup == null) {
+          throw new InterruptedException(e.getMessage());


This is still going to generate an exception in the logs, no? should it just be a log warning?
This is I think too indirect. Why not throw IllegalStateException in createClient instead in this case and catch for it specifically?

srowen · 2019-09-12T13:35:22Z

Also please improve the title of this PR

colinmjj · 2019-09-13T08:12:23Z

@srowen thanks for the comments, I'll update the pr later.

HyukjinKwon · 2019-09-16T00:56:57Z

@colinmjj, also please fill other items in PR description.

SparkQA · 2019-09-16T21:54:18Z

Test build #4871 has finished for PR 25759 at commit b2510fb.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

colinmjj · 2019-09-17T03:12:56Z

@srowen The patch is updated, for the exception from task after executor.stop, add exception process to deal with it.

srowen · 2019-09-17T13:45:19Z

common/network-common/src/test/java/org/apache/spark/network/TransportClientFactorySuite.java

+    try {
+      // NullPointerException occurred if factory.createClient() after factory.close()
+      factory.createClient(TestUtils.getLocalHost(), server1.getPort());
+    } catch (Exception e) {


Why not just catch NullPointerException and ignore it?
But, maybe createClient should throw a better exception to begin with?

The test shows exception is occurred if TransportClientFactory.createClient() called after TransportClientFactory.close().
Agree to throw a better exception and the patch is updated. There should be an IOException now.

srowen · 2019-09-17T13:51:36Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+        // The exception will be thrown from the task becauseof the unexpected status,
+        // see: SPARK-19147, here is to process the exception after executor.stop
+        // as the excepted exception.
+        case t: Throwable if !isLocal && env.isStopped =>


CC @jerryshao or @squito perhaps
My question is, if the executor is shut down, can you even report metrics etc, or is it meaningful?

you might succeed, as this will race against the stopping the executor. But you're very likely to trigger more exceptions from execBackend.statusUpdate, so it probably doesn't make sense to try, especially if the whole point of this change is to cut down on scary error msgs during shutdown.

btw I think env.isStopped will need to be volatile for this to work reliably.

@srowen @squito , thanks for the comments, I check the code again and make clearly how metrics & heartbeat work. You're right, report metrics is meaningless after executor.close(), because heartbeat won't work.
Update the pr and the exception will be processed in "case t: Throwable =>" part with log only.

srowen · 2019-09-17T13:52:56Z

core/src/test/scala/org/apache/spark/executor/ExecutorSuite.scala

@@ -246,6 +246,46 @@ class ExecutorSuite extends SparkFunSuite
    heartbeatZeroAccumulatorUpdateTest(false)
  }

+  test("SPARK-19147: Gracefully handle error in task after executor is stopped") {


The purpose of the change is really to avoid stack traces, not assert about the particular error. Would this pass even before this change? I'm just wondering if this is worth testing.

What may be worth testing is whether metrics are updated, which is the real possible behavior change here? or would they already be reported in case of an error?

Remove the case after I make clearly how metrics works, thanks for review.

srowen · 2019-09-18T14:13:29Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

-                case _: NotSerializableException =>
-                  // t is not serializable so just send the stacktrace
-                  val ef = new ExceptionFailure(t, accUpdates, false).withAccums(accums)
+          if (env.isStopped) {


This is looking OK overall, to me. You might be able to avoid most of the diff due to indentation by only adding a single case:

case t: Throwable if env.isStopped => logError(...) case t: Throwable => // unchanged

… stopped

SparkQA · 2019-09-19T16:14:21Z

Test build #4876 has finished for PR 25759 at commit 8fa404c.

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

dongjoon-hyun

+1, LGTM.

srowen · 2019-09-21T12:31:49Z

Merged to master

dongjoon-hyun · 2019-09-21T15:20:16Z

Hi, All.
Can we have this in branch-2.4 since SPARK-19147 is a long standing issue?

srowen · 2019-09-21T19:28:20Z

Although it's 'just' a cosmetic issue with how errors are logged in a sort of corner case, it's also a minor change. I'm fine if you want to back-port to 2.4.

dongjoon-hyun · 2019-09-21T21:35:05Z

Thanks, @srowen . I'll backport this to branch-2.4.

… stopped ### What changes were proposed in this pull request? TransportClientFactory.createClient() is called by task and TransportClientFactory.close() is called by executor. When stop the executor, close() will set workerGroup = null, NPE will occur in createClient which generate many exception in log. For exception occurs after close(), treated it as an expected Exception and transform it to InterruptedException which can be processed by Executor. ### Why are the changes needed? The change can reduce the exception stack trace in log file, and user won't be confused by these excepted exception. ### Does this PR introduce any user-facing change? N/A ### How was this patch tested? New tests are added in TransportClientFactorySuite and ExecutorSuite Closes #25759 from colinmjj/spark-19147. Authored-by: colinma <colinma@tencent.com> Signed-off-by: Sean Owen <sean.owen@databricks.com> (cherry picked from commit 076186e) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun added the SPARK CORE label Sep 11, 2019

colinmjj force-pushed the spark-19147 branch from 1c1011d to b2510fb Compare September 12, 2019 02:01

srowen reviewed Sep 12, 2019

View reviewed changes

colinmjj changed the title ~~[SPARK-19147][CORE] netty throw NPE~~ [SPARK-19147][CORE] Avoid task to throw NPE caused by TransportClientFactory.createClient after executor stop Sep 13, 2019

colinmjj force-pushed the spark-19147 branch from b2510fb to 4add69b Compare September 17, 2019 02:44

colinmjj changed the title ~~[SPARK-19147][CORE] Avoid task to throw NPE caused by TransportClientFactory.createClient after executor stop~~ [SPARK-19147][CORE] Gracefully handle error in task after executor is stopped Sep 17, 2019

srowen reviewed Sep 17, 2019

View reviewed changes

colinmjj force-pushed the spark-19147 branch from 8691dc1 to cb21ae5 Compare September 18, 2019 13:38

srowen reviewed Sep 18, 2019

View reviewed changes

[SPARK-19147][CORE] Gracefully handle error in task after executor is…

8fa404c

… stopped

colinmjj force-pushed the spark-19147 branch from cb21ae5 to 8fa404c Compare September 19, 2019 01:54

srowen approved these changes Sep 19, 2019

View reviewed changes

dongjoon-hyun approved these changes Sep 21, 2019

View reviewed changes

srowen closed this in 076186e Sep 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19147][CORE] Gracefully handle error in task after executor is stopped #25759

[SPARK-19147][CORE] Gracefully handle error in task after executor is stopped #25759

colinmjj commented Sep 11, 2019 •

edited

Loading

srowen commented Sep 11, 2019

colinmjj commented Sep 12, 2019

srowen Sep 12, 2019

srowen commented Sep 12, 2019

colinmjj commented Sep 13, 2019

HyukjinKwon commented Sep 16, 2019

SparkQA commented Sep 16, 2019

colinmjj commented Sep 17, 2019

srowen Sep 17, 2019

colinmjj Sep 18, 2019

srowen Sep 17, 2019

squito Sep 17, 2019

colinmjj Sep 18, 2019

srowen Sep 17, 2019

colinmjj Sep 18, 2019

srowen Sep 18, 2019

colinmjj Sep 19, 2019

SparkQA commented Sep 19, 2019

dongjoon-hyun left a comment

srowen commented Sep 21, 2019

dongjoon-hyun commented Sep 21, 2019

srowen commented Sep 21, 2019

dongjoon-hyun commented Sep 21, 2019

[SPARK-19147][CORE] Gracefully handle error in task after executor is stopped #25759

[SPARK-19147][CORE] Gracefully handle error in task after executor is stopped #25759

Conversation

colinmjj commented Sep 11, 2019 • 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?

srowen commented Sep 11, 2019

colinmjj commented Sep 12, 2019

Choose a reason for hiding this comment

srowen commented Sep 12, 2019

colinmjj commented Sep 13, 2019

HyukjinKwon commented Sep 16, 2019

SparkQA commented Sep 16, 2019

colinmjj commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 19, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

srowen commented Sep 21, 2019

dongjoon-hyun commented Sep 21, 2019

srowen commented Sep 21, 2019

dongjoon-hyun commented Sep 21, 2019

colinmjj commented Sep 11, 2019 •

edited

Loading