[SPARK-2583] ConnectionManager cannot distinguish whether error occurred or not #1490

sarutak · 2014-07-19T01:02:34Z

No description provided.

AmplabJenkins · 2014-07-19T01:06:44Z

Can one of the admins verify this patch?

rxin · 2014-07-19T20:11:24Z

core/src/main/scala/org/apache/spark/network/MessageChunkHeader.scala

@@ -41,6 +42,13 @@ private[spark] class MessageChunkHeader(
      putInt(totalSize).
      putInt(chunkSize).
      putInt(other).
+      put{


How about

put(if (hasError) 1.asInstanceOf[Byte] else 0.asInstanceOf[Byte])

rxin · 2014-07-19T20:13:23Z

Would it be possible to add a unit test for this?

sarutak · 2014-07-19T20:14:11Z

Thanks @rxin I'll try it.

…ing message

… from remote from successfully

sarutak · 2014-07-24T00:46:32Z

I added some tests for this issue.

sarutak · 2014-07-24T15:49:26Z

I added a test case to ConnectionManager.scala for this issue.

JoshRosen · 2014-07-29T01:08:30Z

Jenkins, this is ok to test.

SparkQA · 2014-07-29T01:13:49Z

QA tests have started for PR 1490. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17326/consoleFull

SparkQA · 2014-07-29T01:14:47Z

QA results for PR 1490:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17326/consoleFull

JoshRosen · 2014-07-29T01:27:04Z

The Jenkins build failed because your code failed the automated Scala style checks (see the Spark Code Style Guide). You can run sbt/sbt scalastyle to run these checks locally.

JoshRosen · 2014-07-29T02:08:39Z

Thanks for submitting this patch. I agree that we should propagate errors to senders if errors occurred while processing their messages. The addition of the hasError field in MessageHeader looks fine to me.

It looks like Future supports onFailure callbacks, so I wonder if we should signal errors by having sendMessageReliably check whether an error occurred and signal failure by calling promise.failed. Note that an exception will be thrown if you call Await.result() on a future that fails. Instead of registering an onSuccess callback and detecting failures inside of it, users would have to define an additional onFailure callback or define onComplete and pattern-match on Success or Failure.

Returning a failed future might prevent bugs where users forget to check the hasFailed field, treat the failure message as a valid response, and experience unexpected behavior; instead, their code would either throw an exception or their callback would never run (assuming they never defined onFailure).

@rxin Any thoughts on signaling failure through failed futures vs. the approach in this PR?

rxin · 2014-07-29T06:25:45Z

I looked at this (with some confusion). Yes, I agree it would be great to just signal failure using the promise when an error occurs.

@sarutak do you think you can do that change?

sarutak · 2014-07-29T10:21:21Z

Thank you for you comment @JoshRosen , @rxin . Of course I'll try it.

…r error reporting Use Futures to signal failures, rather than exposing empty messages to user code.

sarutak · 2014-08-04T15:10:16Z

Thanks for your back up @JoshRosen .

JoshRosen · 2014-08-04T20:39:05Z

Thanks for submitting this PR; it only took a little work for me to pick it up. Do you mind closing this issue, since I'll finish this in #1758? Thanks!

sarutak · 2014-08-05T23:28:33Z

O.K. I take over this issue.

@sarutak

This patch modifies the ConnectionManager so that error messages are sent in reply when uncaught exceptions occur during message processing. This prevents message senders from hanging while waiting for an acknowledgment if the remote message processing failed. This is an updated version of sarutak's PR, #1490. The main change is to use Futures / Promises to signal errors. Author: Kousuke Saruta <[email protected]> Author: Josh Rosen <[email protected]> Closes #1758 from JoshRosen/connection-manager-fixes and squashes the following commits: 68620cb [Josh Rosen] Fix test in BlockFetcherIteratorSuite: 83673de [Josh Rosen] Error ACKs should trigger IOExceptions, so catch only those exceptions in the test. b8bb4d4 [Josh Rosen] Fix manager.id vs managerServer.id typo that broke security tests. 659521f [Josh Rosen] Include previous exception when throwing new one a2f745c [Josh Rosen] Remove sendMessageReliablySync; callers can wait themselves. c01c450 [Josh Rosen] Return Try[Message] from sendMessageReliablySync. f1cd1bb [Josh Rosen] Clean up @sarutak's PR #1490 for [SPARK-2583]: ConnectionManager error reporting 7399c6b [Josh Rosen] Merge remote-tracking branch 'origin/pr/1490' into connection-manager-fixes ee91bb7 [Kousuke Saruta] Modified BufferMessage.scala to keep the spark code style 9dfd0d8 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 e7d9aa6 [Kousuke Saruta] rebase to master 326a17f [Kousuke Saruta] Add test cases to ConnectionManagerSuite.scala for SPARK-2583 2a18d6b [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 22d7ebd [Kousuke Saruta] Add test cases to BlockManagerSuite for SPARK-2583 e579302 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 281589c [Kousuke Saruta] Add a test case to BlockFetcherIteratorSuite.scala for fetching block from remote from successfully 0654128 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 ffaa83d [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 12d3de8 [Kousuke Saruta] Added BlockFetcherIteratorSuite.scala 4117b8f [Kousuke Saruta] Modified ConnectionManager to be alble to handle error during processing message 717c9c3 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 6635467 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 e2b8c4a [Kousuke Saruta] Modify to propagete error using ConnectionManager (cherry picked from commit 17caae4) Signed-off-by: Patrick Wendell <[email protected]>

@sarutak

This patch modifies the ConnectionManager so that error messages are sent in reply when uncaught exceptions occur during message processing. This prevents message senders from hanging while waiting for an acknowledgment if the remote message processing failed. This is an updated version of sarutak's PR, #1490. The main change is to use Futures / Promises to signal errors. Author: Kousuke Saruta <[email protected]> Author: Josh Rosen <[email protected]> Closes #1758 from JoshRosen/connection-manager-fixes and squashes the following commits: 68620cb [Josh Rosen] Fix test in BlockFetcherIteratorSuite: 83673de [Josh Rosen] Error ACKs should trigger IOExceptions, so catch only those exceptions in the test. b8bb4d4 [Josh Rosen] Fix manager.id vs managerServer.id typo that broke security tests. 659521f [Josh Rosen] Include previous exception when throwing new one a2f745c [Josh Rosen] Remove sendMessageReliablySync; callers can wait themselves. c01c450 [Josh Rosen] Return Try[Message] from sendMessageReliablySync. f1cd1bb [Josh Rosen] Clean up @sarutak's PR #1490 for [SPARK-2583]: ConnectionManager error reporting 7399c6b [Josh Rosen] Merge remote-tracking branch 'origin/pr/1490' into connection-manager-fixes ee91bb7 [Kousuke Saruta] Modified BufferMessage.scala to keep the spark code style 9dfd0d8 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 e7d9aa6 [Kousuke Saruta] rebase to master 326a17f [Kousuke Saruta] Add test cases to ConnectionManagerSuite.scala for SPARK-2583 2a18d6b [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 22d7ebd [Kousuke Saruta] Add test cases to BlockManagerSuite for SPARK-2583 e579302 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 281589c [Kousuke Saruta] Add a test case to BlockFetcherIteratorSuite.scala for fetching block from remote from successfully 0654128 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 ffaa83d [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 12d3de8 [Kousuke Saruta] Added BlockFetcherIteratorSuite.scala 4117b8f [Kousuke Saruta] Modified ConnectionManager to be alble to handle error during processing message 717c9c3 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 6635467 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 e2b8c4a [Kousuke Saruta] Modify to propagete error using ConnectionManager

sarutak · 2014-08-09T21:54:02Z

This issue is addressed in #1758 .

@sarutak

This patch modifies the ConnectionManager so that error messages are sent in reply when uncaught exceptions occur during message processing. This prevents message senders from hanging while waiting for an acknowledgment if the remote message processing failed. This is an updated version of sarutak's PR, apache#1490. The main change is to use Futures / Promises to signal errors. Author: Kousuke Saruta <[email protected]> Author: Josh Rosen <[email protected]> Closes apache#1758 from JoshRosen/connection-manager-fixes and squashes the following commits: 68620cb [Josh Rosen] Fix test in BlockFetcherIteratorSuite: 83673de [Josh Rosen] Error ACKs should trigger IOExceptions, so catch only those exceptions in the test. b8bb4d4 [Josh Rosen] Fix manager.id vs managerServer.id typo that broke security tests. 659521f [Josh Rosen] Include previous exception when throwing new one a2f745c [Josh Rosen] Remove sendMessageReliablySync; callers can wait themselves. c01c450 [Josh Rosen] Return Try[Message] from sendMessageReliablySync. f1cd1bb [Josh Rosen] Clean up @sarutak's PR apache#1490 for [SPARK-2583]: ConnectionManager error reporting 7399c6b [Josh Rosen] Merge remote-tracking branch 'origin/pr/1490' into connection-manager-fixes ee91bb7 [Kousuke Saruta] Modified BufferMessage.scala to keep the spark code style 9dfd0d8 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 e7d9aa6 [Kousuke Saruta] rebase to master 326a17f [Kousuke Saruta] Add test cases to ConnectionManagerSuite.scala for SPARK-2583 2a18d6b [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 22d7ebd [Kousuke Saruta] Add test cases to BlockManagerSuite for SPARK-2583 e579302 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 281589c [Kousuke Saruta] Add a test case to BlockFetcherIteratorSuite.scala for fetching block from remote from successfully 0654128 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 ffaa83d [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 12d3de8 [Kousuke Saruta] Added BlockFetcherIteratorSuite.scala 4117b8f [Kousuke Saruta] Modified ConnectionManager to be alble to handle error during processing message 717c9c3 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 6635467 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-2583 e2b8c4a [Kousuke Saruta] Modify to propagete error using ConnectionManager

Co-authored-by: Liang-Chi Hsieh <[email protected]>

Modify to propagete error using ConnectionManager

e2b8c4a

rxin reviewed Jul 19, 2014
View reviewed changes

sarutak added 9 commits July 19, 2014 13:16

Merge branch 'master' of git://git.apache.org/spark into SPARK-2583

6635467

Merge branch 'master' of git://git.apache.org/spark into SPARK-2583

717c9c3

Modified ConnectionManager to be alble to handle error during process…

4117b8f

…ing message

Added BlockFetcherIteratorSuite.scala

12d3de8

Merge branch 'master' of git://git.apache.org/spark into SPARK-2583

ffaa83d

Merge branch 'master' of git://git.apache.org/spark into SPARK-2583

0654128

Add a test case to BlockFetcherIteratorSuite.scala for fetching block…

281589c

… from remote from successfully

Merge branch 'master' of git://git.apache.org/spark into SPARK-2583

e579302

Add test cases to BlockManagerSuite for SPARK-2583

22d7ebd

sarutak added 2 commits July 23, 2014 20:58

Merge branch 'master' of git://git.apache.org/spark into SPARK-2583

2a18d6b

Add test cases to ConnectionManagerSuite.scala for SPARK-2583

326a17f

rebase to master

e7d9aa6

sarutak mentioned this pull request Jul 29, 2014

[SPARK-2677][SPARK-2717]BasicBlockFetchIterator#next can wait forever #1619

Closed

sarutak added 2 commits July 30, 2014 04:56

Merge branch 'master' of git://git.apache.org/spark into SPARK-2583

9dfd0d8

Modified BufferMessage.scala to keep the spark code style

ee91bb7

JoshRosen added a commit to JoshRosen/spark that referenced this pull request Aug 4, 2014

Clean up @sarutak's PR apache#1490 for [SPARK-2583]: ConnectionManage…

f1cd1bb

…r error reporting Use Futures to signal failures, rather than exposing empty messages to user code.

JoshRosen mentioned this pull request Aug 4, 2014

[SPARK-2583] ConnectionManager error reporting #1758

Closed

sarutak closed this Aug 9, 2014

kazuyukitanimura pushed a commit to kazuyukitanimura/spark that referenced this pull request Aug 10, 2022

fix DataFrameJoinSuite (apache#1490)

68865e0

Co-authored-by: Liang-Chi Hsieh <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-2583] ConnectionManager cannot distinguish whether error occurred or not #1490

[SPARK-2583] ConnectionManager cannot distinguish whether error occurred or not #1490

sarutak commented Jul 19, 2014

AmplabJenkins commented Jul 19, 2014

rxin Jul 19, 2014

rxin commented Jul 19, 2014

sarutak commented Jul 19, 2014

sarutak commented Jul 24, 2014

sarutak commented Jul 24, 2014

JoshRosen commented Jul 29, 2014

SparkQA commented Jul 29, 2014

SparkQA commented Jul 29, 2014

JoshRosen commented Jul 29, 2014

JoshRosen commented Jul 29, 2014

rxin commented Jul 29, 2014

sarutak commented Jul 29, 2014

sarutak commented Aug 4, 2014

JoshRosen commented Aug 4, 2014

sarutak commented Aug 5, 2014

sarutak commented Aug 9, 2014

[SPARK-2583] ConnectionManager cannot distinguish whether error occurred or not #1490

[SPARK-2583] ConnectionManager cannot distinguish whether error occurred or not #1490

Conversation

sarutak commented Jul 19, 2014

AmplabJenkins commented Jul 19, 2014

rxin Jul 19, 2014

Choose a reason for hiding this comment

rxin commented Jul 19, 2014

sarutak commented Jul 19, 2014

sarutak commented Jul 24, 2014

sarutak commented Jul 24, 2014

JoshRosen commented Jul 29, 2014

SparkQA commented Jul 29, 2014

SparkQA commented Jul 29, 2014

JoshRosen commented Jul 29, 2014

JoshRosen commented Jul 29, 2014

rxin commented Jul 29, 2014

sarutak commented Jul 29, 2014

sarutak commented Aug 4, 2014

JoshRosen commented Aug 4, 2014

sarutak commented Aug 5, 2014

sarutak commented Aug 9, 2014