-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-4204][Core][WebUI] Change Utils.exceptionString to contain the inner exceptions and make the error information in Web UI more friendly #3073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
94f2566
1e50f71
dfb0032
a07057b
ca509d3
176d1e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,10 +32,30 @@ private[spark] class FetchFailedException( | |
| shuffleId: Int, | ||
| mapId: Int, | ||
| reduceId: Int, | ||
| message: String) | ||
| extends Exception(message) { | ||
| message: String, | ||
| cause: Throwable) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you make this default to |
||
| extends Exception(message, cause) { | ||
|
|
||
| def this( | ||
| bmAddress: BlockManagerId, | ||
| shuffleId: Int, | ||
| mapId: Int, | ||
| reduceId: Int, | ||
| message: String) { | ||
| this(bmAddress, shuffleId, mapId, reduceId, message, null) | ||
| } | ||
|
|
||
| def this( | ||
| bmAddress: BlockManagerId, | ||
| shuffleId: Int, | ||
| mapId: Int, | ||
| reduceId: Int, | ||
| cause: Throwable) { | ||
| this(bmAddress, shuffleId, mapId, reduceId, cause.getMessage, cause) | ||
| } | ||
|
|
||
| def toTaskEndReason: TaskEndReason = FetchFailed(bmAddress, shuffleId, mapId, reduceId, message) | ||
| def toTaskEndReason: TaskEndReason = FetchFailed(bmAddress, shuffleId, mapId, reduceId, | ||
| Utils.exceptionString(this)) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,8 @@ import scala.xml.Text | |
|
|
||
| import java.util.Date | ||
|
|
||
| import org.apache.commons.lang3.StringEscapeUtils | ||
|
|
||
| import org.apache.spark.scheduler.StageInfo | ||
| import org.apache.spark.ui.{ToolTips, UIUtils} | ||
| import org.apache.spark.util.Utils | ||
|
|
@@ -195,7 +197,29 @@ private[ui] class FailedStageTable( | |
|
|
||
| override protected def stageRow(s: StageInfo): Seq[Node] = { | ||
| val basicColumns = super.stageRow(s) | ||
| val failureReason = <td valign="middle"><pre>{s.failureReason.getOrElse("")}</pre></td> | ||
| basicColumns ++ failureReason | ||
| val failureReason = s.failureReason.getOrElse("") | ||
| val isMultiline = failureReason.indexOf('\n') >= 0 | ||
| // Display the first line by default | ||
| val failureReasonSummary = StringEscapeUtils.escapeHtml4( | ||
| if (isMultiline) { | ||
| failureReason.substring(0, failureReason.indexOf('\n')) | ||
| } else { | ||
| failureReason | ||
| }) | ||
| val details = if (isMultiline) { | ||
| // scalastyle:off | ||
| <span onclick="this.parentNode.querySelector('.stacktrace-details').classList.toggle('collapsed')" | ||
| class="expand-details"> | ||
| +details | ||
| </span> ++ | ||
| <div class="stacktrace-details collapsed"> | ||
| <pre>{failureReason}</pre> | ||
| </div> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you indent this entire |
||
| // scalastyle:on | ||
| } else { | ||
| "" | ||
| } | ||
| val failureReasonHtml = <td valign="middle">{failureReasonSummary}{details}</td> | ||
| basicColumns ++ failureReasonHtml | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1598,10 +1598,21 @@ private[spark] object Utils extends Logging { | |
|
|
||
| /** Return a nice string representation of the exception, including the stack trace. */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then maybe we should augment this to say that this stack trace recursively includes the causes of the exception |
||
| def exceptionString(e: Throwable): String = { | ||
| if (e == null) "" else exceptionString(getFormattedClassName(e), e.getMessage, e.getStackTrace) | ||
| if (e == null) { | ||
| "" | ||
| } else { | ||
| val stringWriter = new StringWriter() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add a comment here that says something like |
||
| e.printStackTrace(new PrintWriter(stringWriter)) | ||
| stringWriter.toString | ||
| } | ||
| } | ||
|
|
||
| /** Return a nice string representation of the exception, including the stack trace. */ | ||
| /** | ||
| * Return a nice string representation of the exception, including the stack trace. | ||
| * It's only used for backward compatibility. | ||
| * Note: deprecated because it does not include the exception's cause. | ||
| */ | ||
| @deprecated("Use exceptionString(Throwable) instead", "1.2.0") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just remove this since this is not public API. No need to deprecate
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh never mind you use this somewhere else. I'm still not sure if we should deprecate this because now we'll have warning messages when we build Spark. Maybe it's sufficient to just mention in a comment that the stack trace here does not include the cause of the exception.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved this one into |
||
| def exceptionString( | ||
| className: String, | ||
| description: String, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this warrants a comment in the javadoc for this class why we have
stackTraceandfullStackTraceand what the difference is. Just a simple one line explanation would do.