Skip to content

[SPARK-3887] Send stracktrace in ConnectionManager error replies#2741

Closed
JoshRosen wants to merge 2 commits intoapache:masterfrom
JoshRosen:propagate-cm-exceptions-to-sender
Closed

[SPARK-3887] Send stracktrace in ConnectionManager error replies#2741
JoshRosen wants to merge 2 commits intoapache:masterfrom
JoshRosen:propagate-cm-exceptions-to-sender

Conversation

@JoshRosen
Copy link
Contributor

When reporting that a remote error occurred, the ConnectionManager should also log the stacktrace of the remote exception. This PR accomplishes this by sending the remote exception's stacktrace as the payload in the "negative ACK / error message."

@JoshRosen
Copy link
Contributor Author

Thanks to @patmcdonough for suggesting this. /cc @rxin for review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that this could break if we added another message type and used it to report errors. Do you think that I should pre-emptively add error handling code around this?

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

QA tests have started for PR 2741 at commit cef18b3.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

QA tests have finished for PR 2741 at commit cef18b3.

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

@AmplabJenkins
Copy link

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

@zsxwing
Copy link
Member

zsxwing commented Oct 10, 2014

It's really helpful 👍 Just a small question: Should specify charset when converting between bytes and String, or Spark always assumes the environment uses UTF8?

@rxin
Copy link
Contributor

rxin commented Oct 10, 2014

Changes lgtm overall.

@JoshRosen
Copy link
Contributor Author

Should specify charset when converting between bytes and String, or Spark always assumes the environment uses UTF8?

It looks like String.getBytes uses the platform's default charset, so it's probably safer to explicitly specify a charset. I've updated the patch to do this.

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

QA tests have started for PR 2741 at commit b5366cc.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

QA tests have finished for PR 2741 at commit b5366cc.

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

@AmplabJenkins
Copy link

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

@zsxwing
Copy link
Member

zsxwing commented Oct 12, 2014

LGTM. Is it worth to add val UTF8 = Charset.forName("UTF-8") into object Utils?

@rxin
Copy link
Contributor

rxin commented Oct 12, 2014

It is probably good to add, but maybe in a separate PR?

@rxin
Copy link
Contributor

rxin commented Oct 12, 2014

I'm merging this in master.

@asfgit asfgit closed this in 18bd67c Oct 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants