You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/08/30 05:35:03 UTC
[GitHub] [spark] warrenzhu25 opened a new pull request, #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
warrenzhu25 opened a new pull request, #37716:
URL: https://github.com/apache/spark/pull/37716
### What changes were proposed in this pull request?
Randomize the orders of peer in BlockManagerDecommissioner
### Why are the changes needed?
Avoid migrating data to same nodes
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Manually tested
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a diff in pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #37716:
URL: https://github.com/apache/spark/pull/37716#discussion_r963750642
##########
core/src/main/scala/org/apache/spark/storage/BlockManagerDecommissioner.scala:
##########
@@ -287,7 +287,8 @@ private[storage] class BlockManagerDecommissioner(
val livePeerSet = bm.getPeers(false).toSet
val currentPeerSet = migrationPeers.keys.toSet
val deadPeers = currentPeerSet.diff(livePeerSet)
- val newPeers = livePeerSet.diff(currentPeerSet)
+ // Randomize the orders of the peers to avoid migrating data to same nodes
+ val newPeers = Utils.randomize(livePeerSet.diff(currentPeerSet))
Review Comment:
`livePeerSet.diff(currentPeerSet)` only returns new nodes. It doesn't seem to we'd migrate data to the same node here, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37716:
URL: https://github.com/apache/spark/pull/37716#issuecomment-1231445442
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a diff in pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #37716:
URL: https://github.com/apache/spark/pull/37716#discussion_r964498545
##########
core/src/main/scala/org/apache/spark/storage/BlockManagerDecommissioner.scala:
##########
@@ -287,7 +287,8 @@ private[storage] class BlockManagerDecommissioner(
val livePeerSet = bm.getPeers(false).toSet
val currentPeerSet = migrationPeers.keys.toSet
val deadPeers = currentPeerSet.diff(livePeerSet)
- val newPeers = livePeerSet.diff(currentPeerSet)
+ // Randomize the orders of the peers to avoid migrating data to same nodes
Review Comment:
I think it's still possible to migrate to the same node but just different order, so maybe change the comment a bit as:
```suggestion
// Randomize the orders of the peers to avoid hotspot nodes.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a diff in pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #37716:
URL: https://github.com/apache/spark/pull/37716#discussion_r964495862
##########
core/src/main/scala/org/apache/spark/storage/BlockManagerDecommissioner.scala:
##########
@@ -287,7 +287,8 @@ private[storage] class BlockManagerDecommissioner(
val livePeerSet = bm.getPeers(false).toSet
val currentPeerSet = migrationPeers.keys.toSet
val deadPeers = currentPeerSet.diff(livePeerSet)
- val newPeers = livePeerSet.diff(currentPeerSet)
+ // Randomize the orders of the peers to avoid migrating data to same nodes
+ val newPeers = Utils.randomize(livePeerSet.diff(currentPeerSet))
Review Comment:
I see. You're talking about the orders for two different decommissioning nodes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] warrenzhu25 commented on pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on PR #37716:
URL: https://github.com/apache/spark/pull/37716#issuecomment-1240946554
@Ngone51 Thanks for reviewing this. Will you merge this into master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun closed pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
URL: https://github.com/apache/spark/pull/37716
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] warrenzhu25 commented on pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on PR #37716:
URL: https://github.com/apache/spark/pull/37716#issuecomment-1231976921
@dongjoon-hyun Could you help take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a diff in pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #37716:
URL: https://github.com/apache/spark/pull/37716#discussion_r964498545
##########
core/src/main/scala/org/apache/spark/storage/BlockManagerDecommissioner.scala:
##########
@@ -287,7 +287,8 @@ private[storage] class BlockManagerDecommissioner(
val livePeerSet = bm.getPeers(false).toSet
val currentPeerSet = migrationPeers.keys.toSet
val deadPeers = currentPeerSet.diff(livePeerSet)
- val newPeers = livePeerSet.diff(currentPeerSet)
+ // Randomize the orders of the peers to avoid migrating data to same nodes
Review Comment:
I think it's still possible to migrate to the same node but just with different order, so maybe change the comment a bit as:
```suggestion
// Randomize the orders of the peers to avoid hotspot nodes.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] warrenzhu25 commented on pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on PR #37716:
URL: https://github.com/apache/spark/pull/37716#issuecomment-1238696830
> Thank you for pinging me, @warrenzhu25 . Could you try to add a test case for this please?
if want to verify the `newPeers` is randomized, we might need to expose `newPeers` as `val`. Do you think it's acceptable? Or should we skip this straightforward unit test instead of introducing one method?
I checked other usage of `Utils.randomize` in `ShuffleBlcokFetchIterator`. It seems it also don't have unit test to cover this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] warrenzhu25 commented on a diff in pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on code in PR #37716:
URL: https://github.com/apache/spark/pull/37716#discussion_r964155393
##########
core/src/main/scala/org/apache/spark/storage/BlockManagerDecommissioner.scala:
##########
@@ -287,7 +287,8 @@ private[storage] class BlockManagerDecommissioner(
val livePeerSet = bm.getPeers(false).toSet
val currentPeerSet = migrationPeers.keys.toSet
val deadPeers = currentPeerSet.diff(livePeerSet)
- val newPeers = livePeerSet.diff(currentPeerSet)
+ // Randomize the orders of the peers to avoid migrating data to same nodes
+ val newPeers = Utils.randomize(livePeerSet.diff(currentPeerSet))
Review Comment:
It'll most likely migrate to the same set of nodes as:
1. `bm.getPeers()` always return sorted collection of peers. So if two node start decommission at the close time, they will get the same ordered set of peers.
2. When `currentPeerSet` is empty at the beginning, the `newPeers` is still the same order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a diff in pull request #37716: [SPARK-40269][CORE] Randomize the orders of peer in BlockManagerDecommissioner
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #37716:
URL: https://github.com/apache/spark/pull/37716#discussion_r964498545
##########
core/src/main/scala/org/apache/spark/storage/BlockManagerDecommissioner.scala:
##########
@@ -287,7 +287,8 @@ private[storage] class BlockManagerDecommissioner(
val livePeerSet = bm.getPeers(false).toSet
val currentPeerSet = migrationPeers.keys.toSet
val deadPeers = currentPeerSet.diff(livePeerSet)
- val newPeers = livePeerSet.diff(currentPeerSet)
+ // Randomize the orders of the peers to avoid migrating data to same nodes
Review Comment:
I think it's still possible to migrate to the same node but just in different order, so maybe change the comment a bit as:
```suggestion
// Randomize the orders of the peers to avoid hotspot nodes.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org