[SPARK-19956][Core]Optimize a location order of blocks with topology information #17300
Conversation
|
The fix LGTM, I think it is nice to have such topology priority. CC @mridulm . |
| val getLocations = PrivateMethod[Seq[BlockManagerId]]('getLocations) | ||
| val locations = blockManager invokePrivate getLocations(BroadcastBlockId(0)) | ||
| assert(locations.map(_.host).toSet | ||
| === Set(localHost, localHost, otherHost, otherHost, otherHost)) |
There was a problem hiding this comment.
Remove toSet and make Set as Seq ?
Making it a Set sort of looses the point of this change (ordering is lost).
Same for topology info below below.
| @@ -555,12 +555,15 @@ private[spark] class BlockManager( | |||
|
|
|||
| /** | |||
| * Return a list of locations for the given block, prioritizing the local machine since | |||
| * multiple block managers can share the same host. | |||
| * multiple block managers can share the same host, then try to get the same rack data. | |||
There was a problem hiding this comment.
How about "then try to get the same rack data" -> "followed by hosts on the same rack" ?
|
Hi, @jerryshao @mridulm Thanks for your review, I have updated the code. |
| @@ -497,7 +497,30 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE | |||
| val blockManager = makeBlockManager(128, "exec", bmMaster) | |||
| val getLocations = PrivateMethod[Seq[BlockManagerId]]('getLocations) | |||
| val locations = blockManager invokePrivate getLocations(BroadcastBlockId(0)) | |||
| assert(locations.map(_.host).toSet === Set(localHost, localHost, otherHost)) | |||
| assert(locations.map(_.host) === Seq(localHost, localHost, otherHost)) | |||
There was a problem hiding this comment.
There should also be Seq.
There was a problem hiding this comment.
Looks good to me, will keep it open in case there are other comments.
+CC @jerryshao , @ConeyLiu
|
Ok, thanks a lot. |
|
Hi, @cloud-fan @zsxwing Can you take a look? Thanks a lot. |
|
retest this please |
|
LGTM |
|
Test build #76453 has finished for PR 17300 at commit
|
|
retest this please |
|
Will merge when tests pass. |
|
retest this please |
| */ | ||
| private def getLocations(blockId: BlockId): Seq[BlockManagerId] = { | ||
| val locs = Random.shuffle(master.getLocations(blockId)) | ||
| val (preferredLocs, otherLocs) = locs.partition { loc => blockManagerId.host == loc.host } | ||
| preferredLocs ++ otherLocs | ||
| val (sameRackLocs, differentRackLocs) = otherLocs.partition { | ||
| loc => blockManagerId.topologyInfo == loc.topologyInfo |
There was a problem hiding this comment.
If blockManagerId.topologyInfo is None, we will prefer the locations with empty topologyInfo. It is slightly different with what the shuffling wants to do here.
There was a problem hiding this comment.
Modified, thanks a lot for the good advice.
|
Thanks both of you for review, I have addressed the comments and modified the test case. Please help calling jenkins for test, because I can't trigger that. Thanks again. Also passed in local test. |
|
Test build #76473 has finished for PR 17300 at commit
|
|
retest this please |
1 similar comment
|
retest this please |
|
That is weird, jenkins should have restarted build when I commented ... |
|
Test build #76500 has finished for PR 17300 at commit
|
|
retest this please |
|
Test build #76502 has finished for PR 17300 at commit
|
|
LGTM |
|
Thanks for your review. |
|
thanks, merging to master! |
|
Thanks for merging @cloud-fan, this PR kept dropping form my list ... |
|
Thanks @cloud-fan @mridulm @gatorsmile |
… information ## What changes were proposed in this pull request? When call the method getLocations of BlockManager, we only compare the data block host. Random selection for non-local data blocks, this may cause the selected data block to be in a different rack. So in this patch to increase the sort of the rack. ## How was this patch tested? New test case. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Xianyang Liu <xianyang.liu@intel.com> Closes apache#17300 from ConeyLiu/blockmanager.
What changes were proposed in this pull request?
When call the method getLocations of BlockManager, we only compare the data block host. Random selection for non-local data blocks, this may cause the selected data block to be in a different rack. So in this patch to increase the sort of the rack.
How was this patch tested?
New test case.
Please review http://spark.apache.org/contributing.html before opening a pull request.