sarutak commented on a change in pull request #29082:
URL: https://github.com/apache/spark/pull/29082#discussion_r491983117
##########
File path: core/src/main/scala/org/apache/spark/status/AppStatusListener.scala
##########
@@ -597,6 +598,20 @@ private[spark] class AppStatusListener(
None
}
task.errorMessage = errorMessage
+ task.failureReason = event.reason match {
+ case e: ExceptionFailure =>
Review comment:
As I mention in another place, `message` field in `FailureReason` seems
to be redundant.
If we can remove it away, can we just parse error message and build
`FailureReason` in `computeFailureSummary`?
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -393,6 +394,15 @@ class RuntimeInfo private[spark](
val javaHome: String,
val scalaVersion: String)
+class FailureReason private[spark](
+ val failureType: String,
+ val message: String,
Review comment:
The first line of `stackTrace` contains `message` so I think `message`
is redundant.
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -393,6 +394,15 @@ class RuntimeInfo private[spark](
val javaHome: String,
val scalaVersion: String)
+class FailureReason private[spark](
+ val failureType: String,
+ val message: String,
Review comment:
The first line of `stackTrace` seems to the same with `message` so it's
redundant.
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -282,6 +282,7 @@ class TaskData private[spark](
val speculative: Boolean,
val accumulatorUpdates: Seq[AccumulableInfo],
val errorMessage: Option[String] = None,
+ val failureReason: Option[FailureReason] = None,
Review comment:
`failureReason` could provide duplicate information with `errorMessage`.
If we can parse `errorMassage` and build `FailureSummary` in
`computeFailureSummary`, we don't need this.
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -282,6 +282,7 @@ class TaskData private[spark](
val speculative: Boolean,
val accumulatorUpdates: Seq[AccumulableInfo],
val errorMessage: Option[String] = None,
+ val failureReason: Option[FailureReason] = None,
Review comment:
Existing `errorMessage` and newly added `failureReason` are almost the
same.
Can we avoid adding `failureReason` by building `FailureReason` in
`computeFailureSummary`?
##########
File path: core/src/main/scala/org/apache/spark/status/AppStatusListener.scala
##########
@@ -597,6 +598,20 @@ private[spark] class AppStatusListener(
None
}
task.errorMessage = errorMessage
+ task.failureReason = event.reason match {
Review comment:
As I mentioned in another place, `message` in FailureReason can be
redundant.
If we can remove it, we can just parse error message and build
`FailureReason` in `computeFailureSummary` regardless of `event.reason`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
