[SPARK-20702][Core]TaskContextImpl.markTaskCompleted should not hide the original error #17942
Conversation
| } | ||
| // Call the task completion callbacks. If "markTaskCompleted" is called twice, the second | ||
| // one is no-op. | ||
| context.markTaskCompleted(None) |
There was a problem hiding this comment.
Just add try...finally to wrap this line and fix the style
There was a problem hiding this comment.
We end up calling markTaskCompleted twice when there is an exception thrown, right ?
Perhaps do this one when no Throwable is thrown.
There was a problem hiding this comment.
We end up calling markTaskCompleted twice when there is an exception thrown, right ?
Yes.
Perhaps do this one when no Throwable is thrown.
Then if context.markTaskCompleted(None) throws an exception, context.markTaskFailed(e) will be called, so TaskFailureListener may be called after TaskCompletionListener. This is a slight behavior change. Not sure if it's safe. Someone may depend on the order of calling listeners?
There was a problem hiding this comment.
What I meant was, when there is an exception is throw, there will be two invocations of context.markTaskCompleted.
One with Throwable passed in, and another with None.
This would be confusing to the listeners - no ?
There was a problem hiding this comment.
@mridulm there is a completed flag in markTaskCompleted.
| extends RuntimeException { | ||
|
|
||
| override def getMessage: String = { | ||
| if (errorMessages.size == 1) { |
There was a problem hiding this comment.
The problem here is
if (...) { ... } else {...} +
{...}
is equivalent to
if (...) { ... } else { {...} + {...} }
which is not the intention.
There was a problem hiding this comment.
Good catch !
Btw, file name should probably have been taskListeners.scala (unrelated to this PR) ?
There was a problem hiding this comment.
I guess it's because it's not a class name. cc @rxin since you added this file.
There was a problem hiding this comment.
Aside: Was just curious about the naming - interesting. Is this common pattern in spark code ?
There was a problem hiding this comment.
It's a common pattern in scala.
|
Test build #76762 has finished for PR 17942 at commit
|
|
LGTM. Should merge this soon. @mridulm any thoughts. |
| memoryManager.synchronized { memoryManager.notifyAll() } | ||
| } | ||
| // Call the task completion callbacks. If "markTaskCompleted" is called twice, the second | ||
| // one is no-op. |
There was a problem hiding this comment.
Missed this comment.
LGTM. Thanks for clarifying @zsxwing
|
Thanks! Merging to master and 2.2. |
… the original error ## What changes were proposed in this pull request? This PR adds an `error` parameter to `TaskContextImpl.markTaskCompleted` to propagate the original error. It also fixes an issue that `TaskCompletionListenerException.getMessage` doesn't include `previousError`. ## How was this patch tested? New unit tests. Author: Shixiong Zhu <[email protected]> Closes #17942 from zsxwing/SPARK-20702. (cherry picked from commit 7d6ff39) Signed-off-by: Shixiong Zhu <[email protected]>
… the original error ## What changes were proposed in this pull request? This PR adds an `error` parameter to `TaskContextImpl.markTaskCompleted` to propagate the original error. It also fixes an issue that `TaskCompletionListenerException.getMessage` doesn't include `previousError`. ## How was this patch tested? New unit tests. Author: Shixiong Zhu <[email protected]> Closes apache#17942 from zsxwing/SPARK-20702.
… the original error ## What changes were proposed in this pull request? This PR adds an `error` parameter to `TaskContextImpl.markTaskCompleted` to propagate the original error. It also fixes an issue that `TaskCompletionListenerException.getMessage` doesn't include `previousError`. ## How was this patch tested? New unit tests. Author: Shixiong Zhu <[email protected]> Closes apache#17942 from zsxwing/SPARK-20702.
5 participants
What changes were proposed in this pull request?
This PR adds an
errorparameter toTaskContextImpl.markTaskCompletedto propagate the original error.It also fixes an issue that
TaskCompletionListenerException.getMessagedoesn't includepreviousError.How was this patch tested?
New unit tests.