[SPARK-15736][CORE] Gracefully handle loss of DiskStore files #13473
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @andrewor14 |
|
LGTM. Have you double checked that these are all the places where we do this? |
|
@andrewor14, yeah, I think this is the complete set of locations in Spark 2.x. |
|
Test build #59880 has finished for PR 13473 at commit
|
|
Test build #59884 has finished for PR 13473 at commit
|
asfgit
pushed a commit
that referenced
this pull request
Jun 3, 2016
If an RDD partition is cached on disk and the DiskStore file is lost, then reads of that cached partition will fail and the missing partition is supposed to be recomputed by a new task attempt. In the current BlockManager implementation, however, the missing file does not trigger any metadata updates / does not invalidate the cache, so subsequent task attempts will be scheduled on the same executor and the doomed read will be repeatedly retried, leading to repeated task failures and eventually a total job failure. In order to fix this problem, the executor with the missing file needs to properly mark the corresponding block as missing so that it stops advertising itself as a cache location for that block. This patch fixes this bug and adds an end-to-end regression test (in `FailureSuite`) and a set of unit tests (`in BlockManagerSuite`). Author: Josh Rosen <[email protected]> Closes #13473 from JoshRosen/handle-missing-cache-files. (cherry picked from commit 229f902) Signed-off-by: Andrew Or <[email protected]>
|
Merging into master 2.0. |
asfgit
pushed a commit
that referenced
this pull request
Jun 3, 2016
…iles If an RDD partition is cached on disk and the DiskStore file is lost, then reads of that cached partition will fail and the missing partition is supposed to be recomputed by a new task attempt. In the current BlockManager implementation, however, the missing file does not trigger any metadata updates / does not invalidate the cache, so subsequent task attempts will be scheduled on the same executor and the doomed read will be repeatedly retried, leading to repeated task failures and eventually a total job failure. In order to fix this problem, the executor with the missing file needs to properly mark the corresponding block as missing so that it stops advertising itself as a cache location for that block. This patch fixes this bug and adds an end-to-end regression test (in `FailureSuite`) and a set of unit tests (`in BlockManagerSuite`). This is a branch-1.6 backport of #13473. Author: Josh Rosen <[email protected]> Closes #13479 from JoshRosen/handle-missing-cache-files-branch-1.6.
zzcclp
pushed a commit
to zzcclp/spark
that referenced
this pull request
Jun 3, 2016
…iles If an RDD partition is cached on disk and the DiskStore file is lost, then reads of that cached partition will fail and the missing partition is supposed to be recomputed by a new task attempt. In the current BlockManager implementation, however, the missing file does not trigger any metadata updates / does not invalidate the cache, so subsequent task attempts will be scheduled on the same executor and the doomed read will be repeatedly retried, leading to repeated task failures and eventually a total job failure. In order to fix this problem, the executor with the missing file needs to properly mark the corresponding block as missing so that it stops advertising itself as a cache location for that block. This patch fixes this bug and adds an end-to-end regression test (in `FailureSuite`) and a set of unit tests (`in BlockManagerSuite`). This is a branch-1.6 backport of apache#13473. Author: Josh Rosen <[email protected]> Closes apache#13479 from JoshRosen/handle-missing-cache-files-branch-1.6. (cherry picked from commit 4259a28)
| private def handleLocalReadFailure(blockId: BlockId): Nothing = { | ||
| releaseLock(blockId) | ||
| // Remove the missing block so that its unavailability is reported to the driver | ||
| removeBlock(blockId) |
There was a problem hiding this comment.
Should this be called before the releaseLock() call ?
There was a problem hiding this comment.
No, I don't think so: internally, removeBlock acquires a write lock on the block, so if we called it before the releaseLock call then we'd be calling it while holding a read lock which would cause us to deadlock.
There was a problem hiding this comment.
Looking at BlockInfoManager#lockForWriting(), I think you're right.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If an RDD partition is cached on disk and the DiskStore file is lost, then reads of that cached partition will fail and the missing partition is supposed to be recomputed by a new task attempt. In the current BlockManager implementation, however, the missing file does not trigger any metadata updates / does not invalidate the cache, so subsequent task attempts will be scheduled on the same executor and the doomed read will be repeatedly retried, leading to repeated task failures and eventually a total job failure.
In order to fix this problem, the executor with the missing file needs to properly mark the corresponding block as missing so that it stops advertising itself as a cache location for that block.
This patch fixes this bug and adds an end-to-end regression test (in
FailureSuite) and a set of unit tests (in BlockManagerSuite).