[SPARK-4005][CORE] handle message replies in receive instead of in the individual private methods #2853
Conversation
…e individual private methods
|
QA tests have started for PR 2853 at commit
|
|
QA tests have finished for PR 2853 at commit
|
| if (!blockManagerInfo.contains(blockManagerId)) { | ||
| if (blockManagerId.isDriver && !isLocal) { | ||
| // We intentionally do not register the master (except in local mode), | ||
| // so we should not indicate failure. | ||
| sender ! true | ||
| // do nothing here, "updated == true". |
There was a problem hiding this comment.
Why not just return true here, and return false in the other branch so that we can eliminate the mutable updated variable?
|
Just noticed this old PR while trying to clear the PR review backlog. This seems like a good change and I left a couple of comments on how we could avoid introducing mutability. If you have a chance to update this, that would be great; otherwise, just comment to let me know if you don't have time to do it and I can take care of these fixes myself. Thanks! |
|
@JoshRosen , thank you for your comments, I'll update it soon. |
|
@JoshRosen , code updated according to your comments, can you have a look? |
| // individual private methods. | ||
| updateBlockInfo(blockManagerId, blockId, storageLevel, deserializedSize, size, tachyonSize) | ||
| sender ! updateBlockInfo( | ||
| blockManagerId, blockId, storageLevel, deserializedSize, size, tachyonSize) |
There was a problem hiding this comment.
Super-minor nit, but could you change this line so that it's indented two spaces from the previous one instead of four?
|
LGTM; thanks! I'll merge this in a little bit. |
|
Test build #24155 has finished for PR 2853 at commit
|
|
Test build #24157 has finished for PR 2853 at commit
|
|
It occurred to me that we should probably document this method's return value. |
| } | ||
| return | ||
| return true |
There was a problem hiding this comment.
Actually, I think you should just remove this line since it's unreachable.
There was a problem hiding this comment.
yes, remove the line will be the same logic with the original one.
|
@JoshRosen , thanks for your detailed review. |
|
jenkins, retest this please |
|
Jenkins, retest this please. I think that this Jenkins failure was due to one of the machines' disks filling up; I've taken that machine offline while we investigate. |
|
Test build #541 has finished for PR 2853 at commit
|
|
LGTM. Since this is code-cleanup and not a bugfix, I'm only going to merge this into |
In BlockManagermasterActor, when handling message type UpdateBlockInfo, the message replies is in handled in individual private methods, should handle it in receive of Akka.