[SPARK-30387] Improving stop hook log message #27049
Conversation
| @@ -164,7 +164,7 @@ private[spark] class YarnClientSchedulerBackend( | |||
|
|
|||
| super.stop() | |||
| client.stop() | |||
| logInfo("Stopped") | |||
| logInfo("YarnClientSchedulerBackend Stopped") | |||
There was a problem hiding this comment.
Isn't YarnClientSchedulerBackend added to full of log message, as in most case the format contains the logger name (mostly same as class name)?
There was a problem hiding this comment.
Current log message is
2019-12-27 20:23:26,613 | INFO | [main] | Stopped | org.apache.spark.internal.Logging$class.logInfo(Logging.scala:54)
There was a problem hiding this comment.
You must be using a custom logging file because by default you get:
19/12/31 10:47:28 INFO YarnClientSchedulerBackend: Stopped
There was a problem hiding this comment.
Look at the spark conf/log4j.properties.template file for the format used there
There was a problem hiding this comment.
If true, and I suspect you're right, is there much harm in just adding to the log message anyway?
There was a problem hiding this comment.
Also on other places we are using descriptive log message. Like,
There was a problem hiding this comment.
no harm, I'm ok with the change, there may just be other places in the code that aren't clear without it so you may want to look at your logging config anyway.
There was a problem hiding this comment.
Same as @tgravescs, I think log4j format matters, but agree no harm with this.
There was a problem hiding this comment.
I don't think we should necessarily add it.
because by default you get:
19/12/31 10:47:28 INFO YarnClientSchedulerBackend: Stopped
Also, by default, seems showing the logs fine. Otherwise, we should fix so many places as @tgravescs pointed out. If class names should be shown, that should properly configured for such places.
There was a problem hiding this comment.
Yes. as @tgravescs pointed out, here logs can be improved by changing log4j conversion pattern.
|
Jenkins test this please |
|
Test build #115993 has finished for PR 27049 at commit
|
| @@ -164,7 +164,7 @@ private[spark] class YarnClientSchedulerBackend( | |||
|
|
|||
| super.stop() | |||
| client.stop() | |||
| logInfo("Stopped") | |||
| logInfo("YarnClientSchedulerBackend Stopped") | |||
There was a problem hiding this comment.
@amanomer, why don't you just make the log better just by not using class name directly like the reference you pointed out? e.g.) YARN client scheduler backend stopped.
There was a problem hiding this comment.
@jobitmathew please update the log message.
There was a problem hiding this comment.
@amanomer
@HyukjinKwon
@srowen
@tgravescs
changes done as per HyukjinKwon's suggestion.Could you please check.
|
Test build #4981 has finished for PR 27049 at commit
|
|
Merged to master |
What changes were proposed in this pull request?
ShutdownHook of YarnClientSchedulerBackend prints just "Stopped" which can be improved to "YarnClientSchedulerBackend Stopped" for better understanding.
Why are the changes needed?
While stopping or gracefully exiting the spark-shell/spark-sql --master yarn, only printing
stoppedis useless.Does this PR introduce any user-facing change?
Yes. Log info message change.
How was this patch tested?
Manually