[SPARK-35234][CORE] Reserve the format of stage failureMessage #32356
Conversation
|
There is another case of using What about fixing that one too? |
|
SGTM |
| |Most recent failure reason: $message | ||
| """.stripMargin.replaceAll("\n", " ") | ||
| s"$failedStage (${failedStage.name}) has failed the maximum allowable number of " + | ||
| s"times: $maxConsecutiveStageAttempts. Most recent failure reason: $message" |
There was a problem hiding this comment.
Note that we don't append \n for this "Most recent failure reason" because message already contains it:
val message = s"Stage failed because barrier task $task finished unsuccessfully.\n" +
failure.toErrorString
|
@Ngone51 the images you attached to the PR description are the logs of a running test? But I think these messages are landing on the UI and those should be attached to the PR description. |
|
Yes, the screenshot is from a test. It's for convenient purpose. But the stage failure is shown to users directly, right? So I consider it a user-facing change. Attach UI changes sounds like better idea, which is surely a user-facing change. I'll try. |
|
@attilapiros I have done the experiment on UI. And it turns out that the format in UI is always correct. That's because we passed the formatted |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
The description would be perfect with the UI screenshot and it would be so good to take a look how the message is actually rendered on UI. @Ngone51 So I would like to kindly ask you to make those screenshots (before and after) by for example doing a temporarily code change! |
|
Test build #137977 has finished for PR 32356 at commit
|
|
@attilapiros I'm not sure if I misunderstood your request. I have done the UI experiment (#32356 (comment)) and it turns out UI isn't affected. So I think I don't have to add the UI screenshot. |
|
@Ngone51 Oh I see. Then please update the "Does this PR introduce any user-facing change?" section to "No." As users cannot see / should not care about test logs. |
|
In the title you can add [TESTS]. |
@attilapiros It's not only able to see in the test, but also in a user application. Here, I used the test as an example to show the difference. And probably it's my bad not to mention in advance: I changed a little bit in the test in order to show the error message (it's captured previously). So, I'm not fixing the test. |
|
I think I see what you mean and why I am confused: I know you are not fixing a test but as you used test logs and said the UI is not affected I thought it has only relevance for test execution. But of course this fixes the application log as well. So let's mention "application log" in the PR description to help the others who are reading the commit message after the merge. |
|
changes look fine to me |
|
I leave it here for 2 more days to let the others to review it and if no issue comes up I'll merge it (assuming it's still passing CI and no review is in progress). |
|
Will leave it to you @attilapiros then :-) |
|
@HyukjinKwon I am keeping my promise made here: #32180 (comment) |
|
thanks all! |
What changes were proposed in this pull request?
failureMessageis already formatted, butreplaceAll("\n", " ")destroyed the format. This PR fixed it.Why are the changes needed?
The formatted error message is easier to read and debug.
Does this PR introduce any user-facing change?
Yes, users see the clear error message in the application log.
(Note I changed a little bit to let the test throw exception intentionally. The test itself is good.)
Before:
After:
How was this patch tested?
Manually tested.