[SPARK-10381] Fix mixup of taskAttemptNumber & attemptId in OutputCommitCoordinator (branch-1.3 backport)#8790
[SPARK-10381] Fix mixup of taskAttemptNumber & attemptId in OutputCommitCoordinator (branch-1.3 backport)#8790JoshRosen wants to merge 4 commits into
Conversation
…mitCoordinator When speculative execution is enabled, consider a scenario where the authorized committer of a particular output partition fails during the OutputCommitter.commitTask() call. In this case, the OutputCommitCoordinator is supposed to release that committer's exclusive lock on committing once that task fails. However, due to a unit mismatch (we used task attempt number in one place and task attempt id in another) the lock will not be released, causing Spark to go into an infinite retry loop. This bug was masked by the fact that the OutputCommitCoordinator does not have enough end-to-end tests (the current tests use many mocks). Other factors contributing to this bug are the fact that we have many similarly-named identifiers that have different semantics but the same data types (e.g. attemptNumber and taskAttemptId, with inconsistent variable naming which makes them difficult to distinguish). This patch adds a regression test and fixes this bug by always using task attempt numbers throughout this code. Author: Josh Rosen <joshrosen@databricks.com> Closes apache#8544 from JoshRosen/SPARK-10381. (cherry picked from commit 38700ea) Signed-off-by: Josh Rosen <joshrosen@databricks.com>
|
Test build #42561 has finished for PR 8790 at commit
|
|
Test build #42569 has finished for PR 8790 at commit
|
|
Jenkins, retest this please. |
There was a problem hiding this comment.
Maybe we need to take a look at the code to see the meaning of attempt. I find https://github.com/apache/spark/blob/branch-1.3/core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala#L451-L452. Looks like it is indeed attemptNumber.
|
LGTM. We can merge it once jenkins is good. |
|
Test build #42696 timed out for PR 8790 at commit |
|
Jenkins, retest this please. |
|
Test build #42704 has started for PR 8790 at commit
|
|
Test build #42705 has finished for PR 8790 at commit
|
|
Jenkins, retest this please. |
|
Test build #42715 has finished for PR 8790 at commit
|
|
Jenkins, retest this please. |
|
Test build #42763 timed out for PR 8790 at commit |
|
test this please |
|
It looks like all of the tests passed on this last run; it just timed out during MiMa checks while fetching some old versions of dependencies. The tests that failed in earlier runs are known to be flaky. Given this, I'm going to merge this now. |
…mitCoordinator (branch-1.3 backport) This is a backport of #8544 to `branch-1.3` for inclusion in 1.3.2. Author: Josh Rosen <joshrosen@databricks.com> Closes #8790 from JoshRosen/SPARK-10381-1.3.
|
Test build #42850 has finished for PR 8790 at commit
|
|
Test FAILed. |
5 participants
This is a backport of #8544 to
branch-1.3for inclusion in 1.3.2.