Skip to content

[SPARK-27496][CORE] Fatal errors should also be sent back to the sender - #24396

Closed
zsxwing wants to merge 1 commit into
apache:masterfrom
zsxwing:SPARK-27496
Closed

[SPARK-27496][CORE] Fatal errors should also be sent back to the sender#24396
zsxwing wants to merge 1 commit into
apache:masterfrom
zsxwing:SPARK-27496

Conversation

@zsxwing

@zsxwing zsxwing commented Apr 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

When a fatal error (such as StackOverflowError) throws from "receiveAndReply", we should try our best to notify the sender. Otherwise, the sender will hang until timeout.

In addition, when a MessageLoop is dying unexpectedly, it should resubmit a new one so that Dispatcher is still working.

How was this patch tested?

New unit tests.

@zsxwing

zsxwing commented Apr 17, 2019

Copy link
Copy Markdown
Member Author

cc @jiangxb1987

@SparkQA

SparkQA commented Apr 18, 2019

Copy link
Copy Markdown

Test build #104679 has finished for PR 24396 at commit 0fdf027.

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

jiangxb1987
jiangxb1987 approved these changes Apr 18, 2019
View reviewed changes

@jiangxb1987 jiangxb1987 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

felixcheung
felixcheung reviewed Apr 18, 2019
View reviewed changes
Comment thread core/src/main/scala/org/apache/spark/rpc/netty/Dispatcher.scala
try {
// Re-submit a MessageLoop so that Dispatcher will still work if
// UncaughtExceptionHandler decides to not kill JVM.
threadpool.execute(new MessageLoop)

@felixcheung felixcheung Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should log before calling this?

@zsxwing zsxwing Apr 18, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually don't log if it's going to be re-thrown since it will cause double logs.

Comment thread core/src/main/scala/org/apache/spark/rpc/netty/Dispatcher.scala
// UncaughtExceptionHandler decides to not kill JVM.
threadpool.execute(new MessageLoop)
} finally {
throw t

@felixcheung felixcheung Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this will always rethrow even when the MessageLoop is re-submitted? do you intend this to be not finally but catch?

@zsxwing zsxwing Apr 18, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. This is going to throw the fatal errors (non fatal errors have been caught and should not reach here) to UncaughtExceptionHandler to let it decide what to do.

@dongjoon-hyun

dongjoon-hyun commented Apr 19, 2019

Copy link
Copy Markdown
Member

Retest this please.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-27496][CORE]Fatal errors should also be sent back to the sender [SPARK-27496][CORE] Fatal errors should also be sent back to the sender Apr 19, 2019
@SparkQA

SparkQA commented Apr 19, 2019

Copy link
Copy Markdown

Test build #104759 has finished for PR 24396 at commit 0fdf027.

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

dongjoon-hyun
dongjoon-hyun approved these changes Apr 21, 2019
View reviewed changes

@dongjoon-hyun dongjoon-hyun left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Merged to master/2.4/2.3.
Thank you, @zsxwing , @jinxing64 , @felixcheung .

dongjoon-hyun pushed a commit that referenced this pull request Apr 22, 2019
@zsxwing @dongjoon-hyun
[SPARK-27496][CORE] Fatal errors should also be sent back to the sender
33864a8 
## What changes were proposed in this pull request?

When a fatal error (such as StackOverflowError) throws from "receiveAndReply", we should try our best to notify the sender. Otherwise, the sender will hang until timeout.

In addition, when a MessageLoop is dying unexpectedly, it should resubmit a new one so that Dispatcher is still working.

## How was this patch tested?

New unit tests.

Closes #24396 from zsxwing/SPARK-27496.

Authored-by: Shixiong Zhu <zsxwing@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit 009059e)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
dongjoon-hyun pushed a commit that referenced this pull request Apr 22, 2019
@zsxwing @dongjoon-hyun
[SPARK-27496][CORE] Fatal errors should also be sent back to the sender
257abc4 
## What changes were proposed in this pull request?

When a fatal error (such as StackOverflowError) throws from "receiveAndReply", we should try our best to notify the sender. Otherwise, the sender will hang until timeout.

In addition, when a MessageLoop is dying unexpectedly, it should resubmit a new one so that Dispatcher is still working.

## How was this patch tested?

New unit tests.

Closes #24396 from zsxwing/SPARK-27496.

Authored-by: Shixiong Zhu <zsxwing@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit 009059e)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
@zsxwing
zsxwing deleted the SPARK-27496 branch April 22, 2019 07:24
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Jul 23, 2019
@zsxwing @kai-chi
[SPARK-27496][CORE] Fatal errors should also be sent back to the sender
2d6b2ef 
## What changes were proposed in this pull request?

When a fatal error (such as StackOverflowError) throws from "receiveAndReply", we should try our best to notify the sender. Otherwise, the sender will hang until timeout.

In addition, when a MessageLoop is dying unexpectedly, it should resubmit a new one so that Dispatcher is still working.

## How was this patch tested?

New unit tests.

Closes apache#24396 from zsxwing/SPARK-27496.

Authored-by: Shixiong Zhu <zsxwing@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit 009059e)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Jul 25, 2019
@zsxwing @kai-chi
[SPARK-27496][CORE] Fatal errors should also be sent back to the sender
4633c71 
## What changes were proposed in this pull request?

When a fatal error (such as StackOverflowError) throws from "receiveAndReply", we should try our best to notify the sender. Otherwise, the sender will hang until timeout.

In addition, when a MessageLoop is dying unexpectedly, it should resubmit a new one so that Dispatcher is still working.

## How was this patch tested?

New unit tests.

Closes apache#24396 from zsxwing/SPARK-27496.

Authored-by: Shixiong Zhu <zsxwing@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit 009059e)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Aug 1, 2019
@zsxwing @kai-chi
[SPARK-27496][CORE] Fatal errors should also be sent back to the sender
1a46a84 
## What changes were proposed in this pull request?

When a fatal error (such as StackOverflowError) throws from "receiveAndReply", we should try our best to notify the sender. Otherwise, the sender will hang until timeout.

In addition, when a MessageLoop is dying unexpectedly, it should resubmit a new one so that Dispatcher is still working.

## How was this patch tested?

New unit tests.

Closes apache#24396 from zsxwing/SPARK-27496.

Authored-by: Shixiong Zhu <zsxwing@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit 009059e)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixcheung felixcheung felixcheung left review comments
@jiangxb1987 jiangxb1987 jiangxb1987 approved these changes
@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zsxwing @SparkQA @dongjoon-hyun @jiangxb1987 @felixcheung