You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "rezasafi (via GitHub)" <gi...@apache.org> on 2023/12/14 00:12:36 UTC
[PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
rezasafi opened a new pull request, #44340:
URL: https://github.com/apache/spark/pull/44340
### What changes were proposed in this pull request?
Currently SparkListenerApplicationEnd only has a timestamp value and there is not exit status recorded with it.
This change will exitcode to the SparkListenerApplicationEnd event.
### Why are the changes needed?
Without this it is hard to understand whether an attempt has failed or succeeded when using spark listeners.
### Does this PR introduce _any_ user-facing change?
No, the added exitCode is an optional parameter of the event.
### How was this patch tested?
Locally using the normal tests. also anew tests is added.
### Was this patch authored or co-authored using generative AI tooling?
No
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #44340:
URL: https://github.com/apache/spark/pull/44340#discussion_r1428624893
##########
core/src/main/scala/org/apache/spark/scheduler/SparkListener.scala:
##########
@@ -289,7 +289,8 @@ case class SparkListenerApplicationStart(
driverAttributes: Option[Map[String, String]] = None) extends SparkListenerEvent
@DeveloperApi
-case class SparkListenerApplicationEnd(time: Long) extends SparkListenerEvent
+case class SparkListenerApplicationEnd(time: Long,
+ exitCode: Option[Int] = None) extends SparkListenerEvent
Review Comment:
Fix indentation here.
##########
core/src/main/scala/org/apache/spark/util/JsonProtocol.scala:
##########
@@ -1065,7 +1069,11 @@ private[spark] object JsonProtocol extends JsonUtils {
}
def applicationEndFromJson(json: JsonNode): SparkListenerApplicationEnd = {
- SparkListenerApplicationEnd(json.get("Timestamp").extractLong)
+ var exitCode: Option[Int] = None
+ if(json.has("ExitCode")) {
+ exitCode = Some(json.get("ExitCode").extractInt)
+ }
Review Comment:
```suggestion
val exitCode = jsonOption(json.get("ExitCode")).map(_.extractInt)
```
##########
core/src/main/scala/org/apache/spark/util/JsonProtocol.scala:
##########
@@ -277,6 +277,10 @@ private[spark] object JsonProtocol extends JsonUtils {
g.writeStartObject()
g.writeStringField("Event", SPARK_LISTENER_EVENT_FORMATTED_CLASS_NAMES.applicationEnd)
g.writeNumberField("Timestamp", applicationEnd.time)
+ applicationEnd.exitCode match {
+ case Some(exitCode) => g.writeNumberField("ExitCode", exitCode)
+ case _ => // No exit code provided
+ }
Review Comment:
nit:
```suggestion
// exit code, when available
applicationEnd.exitCode.foreach(exitCode => g.writeNumberField("ExitCode", exitCode))
```
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #44340:
URL: https://github.com/apache/spark/pull/44340#issuecomment-1858687608
The mima test is also failing, can you update the mima rules for `SparkListenerApplicationEnd ` ?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #44340:
URL: https://github.com/apache/spark/pull/44340#issuecomment-1861965125
+CC @thejdeep as well.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "rezasafi (via GitHub)" <gi...@apache.org>.
rezasafi commented on PR #44340:
URL: https://github.com/apache/spark/pull/44340#issuecomment-1863420509
test failures where not related to this change. I rebased to master in case the failures where impact of changes to master
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "rezasafi (via GitHub)" <gi...@apache.org>.
rezasafi commented on PR #44340:
URL: https://github.com/apache/spark/pull/44340#issuecomment-1854890434
@squito @vanzin @attilapiros old friends appreciate your review here :-)
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "rezasafi (via GitHub)" <gi...@apache.org>.
rezasafi commented on code in PR #44340:
URL: https://github.com/apache/spark/pull/44340#discussion_r1431003343
##########
core/src/test/scala/org/apache/spark/scheduler/EventLoggingListenerSuite.scala:
##########
@@ -75,6 +75,10 @@ class EventLoggingListenerSuite extends SparkFunSuite with LocalSparkContext wit
testApplicationEventLogging()
}
+ test("End-to-end event logging with exit code") {
+ testEventLogging(exitCode = Some(0))
Review Comment:
sure, i can add an explicit test, but since default is None, all other tests basically test exitCode = None
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #44340: [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener
URL: https://github.com/apache/spark/pull/44340
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #44340:
URL: https://github.com/apache/spark/pull/44340#discussion_r1430813308
##########
core/src/test/scala/org/apache/spark/scheduler/EventLoggingListenerSuite.scala:
##########
@@ -75,6 +75,10 @@ class EventLoggingListenerSuite extends SparkFunSuite with LocalSparkContext wit
testApplicationEventLogging()
}
+ test("End-to-end event logging with exit code") {
+ testEventLogging(exitCode = Some(0))
Review Comment:
Forgoit to mention it, can you add another case here where `exitCode = None` ?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "rezasafi (via GitHub)" <gi...@apache.org>.
rezasafi commented on PR #44340:
URL: https://github.com/apache/spark/pull/44340#issuecomment-1861166830
Thank you so much for the comments @mridulm. I applied them
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #44340:
URL: https://github.com/apache/spark/pull/44340#issuecomment-1864049616
Merged to master.
Thanks for adding this @rezasafi !
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46399][Core] Add exit status to the Application End event for the use of Spark Listener [spark]
Posted by "rezasafi (via GitHub)" <gi...@apache.org>.
rezasafi commented on PR #44340:
URL: https://github.com/apache/spark/pull/44340#issuecomment-1864857233
Thank you so much @mridulm. Really appreciate your review
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org