-
Notifications
You must be signed in to change notification settings - Fork 28k
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
[SPARK-48325][CORE] Always specify messages in ExecutorRunner.killProcess #46641
Conversation
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.
Looks reasonable to me, but I will let @Ngone51 or @jiangxb1987 take a look at it.
case e: Exception => | ||
logError("Error running executor", e) | ||
state = ExecutorState.FAILED | ||
killProcess(Some(e.toString)) | ||
killProcess(s"Error running executor: $e") |
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.
Could you dump the full stacktrace for the error message?
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.
The message will be sent to Master and Driver. Do you think including the full stacktrace might make it too large? How about logging it in the Worker log here?
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.
logError("Error running executor", e)
This ^ should already log the full stack in the worker log.
My concern is that the exception that throw here could be rare case and full stack will help the debug. Since worker already logs it, I'm fine to return a short error message to master.
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.
+1, LGTM.
Merged to master for Apache Spark 4.0.0. Thank you, @bozhang2820 and all. |
What changes were proposed in this pull request?
This change is to always specify the message in
ExecutorRunner.killProcess
.Why are the changes needed?
This is to get the occurrence rate for different cases when killing the executor process, in order to analyze executor running stability.
Does this PR introduce any user-facing change?
No
How was this patch tested?
N/A
Was this patch authored or co-authored using generative AI tooling?
No