[SPARK-4006] In long running contexts, we encountered the situation of double registe... #2886
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
…ster without a remove in between. The cause for that is unknown, and assumed a temp network issue.
However, since the second register is with a BlockManagerId on a different port, blockManagerInfo.contains() returns false, while blockManagerIdByExecutor returns Some. This inconsistency is caught in a conditional statement that does System.exit(1), which is a huge robustness issue for us.
The fix - simply remove the old id from both maps during register when this happens. We are mimicking the behavior of expireDeadHosts(), by doing local cleanup of the maps before trying to add new ones.
Also - added some logging for register and unregister.
|
Can one of the admins verify this patch? |
|
ok to test |
|
QA tests have started for PR 2886 at commit
|
|
QA tests have finished for PR 2886 at commit
|
|
Test PASSed. |
| // This should never happen. Let's just quit. | ||
| logError("Got two different block manager registrations on " + id.executorId) | ||
| System.exit(1) | ||
| // A block manager of the same executor already exists so remove it (assumed dead). |
There was a problem hiding this comment.
already exists, so remove it
There was a problem hiding this comment.
actually what I meant was to add a comma between "exists" and "so"... It's ok I can fix this myself when I merge it
|
Hey @tsliwowicz there are a few style and wording issues that I'd like to see fixed. Also, I would prefer to have the whitespace changes reverted. However, I think the fix is correct and this LGTM logically. |
|
QA tests have started for PR 2886 at commit
|
|
@andrewor14 - thanks for the comments. I believe I fixed them all. Let me know! |
|
QA tests have started for PR 2886 at commit
|
|
Test FAILed. |
|
the failure seems technical (not related to my fix), I think. Local maven build works fine for me. |
|
QA tests have finished for PR 2886 at commit
|
|
Test PASSed. |
|
QA tests have finished for PR 2886 at commit
|
|
Test PASSed. |
|
Ok I have merged this into master. It doesn't merge cleanly into 1.1, so @tsliwowicz can you create a new PR against that branch? Thanks. |
|
(also branch 1.0) |
tsliwowicz
added a commit
to taboola/spark
that referenced
this pull request
Oct 23, 2014
…f double registe... ...r without a remove in between. The cause for that is unknown, and assumed a temp network issue. However, since the second register is with a BlockManagerId on a different port, blockManagerInfo.contains() returns false, while blockManagerIdByExecutor returns Some. This inconsistency is caught in a conditional statement that does System.exit(1), which is a huge robustness issue for us. The fix - simply remove the old id from both maps during register when this happens. We are mimicking the behavior of expireDeadHosts(), by doing local cleanup of the maps before trying to add new ones. Also - added some logging for register and unregister. This is just like apache#2854 except it's on master Author: Tal Sliwowicz <tal.s@taboola.com> Closes apache#2886 from tsliwowicz/master-block-mgr-removal and squashes the following commits: 094d508 [Tal Sliwowicz] some more white space change undone 41a2217 [Tal Sliwowicz] some more whitspaces change undone 7bcfc3d [Tal Sliwowicz] whitspaces fix df9d98f [Tal Sliwowicz] Code review comments fixed f48bce9 [Tal Sliwowicz] In long running contexts, we encountered the situation of double register without a remove in between. The cause for that is unknown, and assumed a temp network issue. (cherry picked from commit 6b48522) Conflicts: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala
tsliwowicz
added a commit
to taboola/spark
that referenced
this pull request
Oct 23, 2014
…f double registe... ...r without a remove in between. The cause for that is unknown, and assumed a temp network issue. However, since the second register is with a BlockManagerId on a different port, blockManagerInfo.contains() returns false, while blockManagerIdByExecutor returns Some. This inconsistency is caught in a conditional statement that does System.exit(1), which is a huge robustness issue for us. The fix - simply remove the old id from both maps during register when this happens. We are mimicking the behavior of expireDeadHosts(), by doing local cleanup of the maps before trying to add new ones. Also - added some logging for register and unregister. This is just like apache#2854 except it's on master Author: Tal Sliwowicz <tal.s@taboola.com> Closes apache#2886 from tsliwowicz/master-block-mgr-removal and squashes the following commits: 094d508 [Tal Sliwowicz] some more white space change undone 41a2217 [Tal Sliwowicz] some more whitspaces change undone 7bcfc3d [Tal Sliwowicz] whitspaces fix df9d98f [Tal Sliwowicz] Code review comments fixed f48bce9 [Tal Sliwowicz] In long running contexts, we encountered the situation of double register without a remove in between. The cause for that is unknown, and assumed a temp network issue. (cherry picked from commit 6b48522) Conflicts: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala (cherry picked from commit d122236) Conflicts: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala
tsliwowicz
added a commit
to taboola/spark
that referenced
this pull request
Oct 23, 2014
…f double registe... ...r without a remove in between. The cause for that is unknown, and assumed a temp network issue. However, since the second register is with a BlockManagerId on a different port, blockManagerInfo.contains() returns false, while blockManagerIdByExecutor returns Some. This inconsistency is caught in a conditional statement that does System.exit(1), which is a huge robustness issue for us. The fix - simply remove the old id from both maps during register when this happens. We are mimicking the behavior of expireDeadHosts(), by doing local cleanup of the maps before trying to add new ones. Also - added some logging for register and unregister. This is just like apache#2854 except it's on master Author: Tal Sliwowicz <tal.s@taboola.com> Closes apache#2886 from tsliwowicz/master-block-mgr-removal and squashes the following commits: 094d508 [Tal Sliwowicz] some more white space change undone 41a2217 [Tal Sliwowicz] some more whitspaces change undone 7bcfc3d [Tal Sliwowicz] whitspaces fix df9d98f [Tal Sliwowicz] Code review comments fixed f48bce9 [Tal Sliwowicz] In long running contexts, we encountered the situation of double register without a remove in between. The cause for that is unknown, and assumed a temp network issue. (cherry picked from commit 6b48522) Conflicts: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala
|
will do. Can you also merge into the 0.9 branch? I will update the PR I already have for it. #2854 |
This was referenced Oct 23, 2014
|
@andrewor14 I created PR for 1.0, 1.1 and updated the 0.9 PR - can you please review and merge if ok? Non of the merges were clean, so I decided to do it for each branch. |
|
Thanks a lot @tsliwowicz I'll do that shortly. |
asfgit
pushed a commit
that referenced
this pull request
Oct 24, 2014
…f d... ...ouble registe... ...r without a remove in between. The cause for that is unknown, and assumed a temp network issue. However, since the second register is with a BlockManagerId on a different port, blockManagerInfo.contains() returns false, while blockManagerIdByExecutor returns Some. This inconsistency is caught in a conditional statement that does System.exit(1), which is a huge robustness issue for us. The fix - simply remove the old id from both maps during register when this happens. We are mimicking the behavior of expireDeadHosts(), by doing local cleanup of the maps before trying to add new ones. Also - added some logging for register and unregister. This is just like #2886 except it's on branch-1.1 Author: Tal Sliwowicz <tal.s@taboola.com> Closes #2915 from tsliwowicz/branch-1.1-block-mgr-removal and squashes the following commits: d122236 [Tal Sliwowicz] [SPARK-4006] In long running contexts, we encountered the situation of double registe...
asfgit
pushed a commit
that referenced
this pull request
Dec 18, 2014
…f d... ...ouble registe... ...r without a remove in between. The cause for that is unknown, and assumed a temp network issue. However, since the second register is with a BlockManagerId on a different port, blockManagerInfo.contains() returns false, while blockManagerIdByExecutor returns Some. This inconsistency is caught in a conditional statement that does System.exit(1), which is a huge robustness issue for us. The fix - simply remove the old id from both maps during register when this happens. We are mimicking the behavior of expireDeadHosts(), by doing local cleanup of the maps before trying to add new ones. Also - added some logging for register and unregister. This is just like #2886 except it's on branch-1.0 Author: Tal Sliwowicz <tal.s@taboola.com> Closes #2914 from tsliwowicz/branch-1.0-block-mgr-removal and squashes the following commits: 1014493 [Tal Sliwowicz] [SPARK-4006] In long running contexts, we encountered the situation of double registe...
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.
...r without a remove in between. The cause for that is unknown, and assumed a temp network issue.
However, since the second register is with a BlockManagerId on a different port, blockManagerInfo.contains() returns false, while blockManagerIdByExecutor returns Some. This inconsistency is caught in a conditional statement that does System.exit(1), which is a huge robustness issue for us.
The fix - simply remove the old id from both maps during register when this happens. We are mimicking the behavior of expireDeadHosts(), by doing local cleanup of the maps before trying to add new ones.
Also - added some logging for register and unregister.
This is just like #2854 except it's on master