Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/17325#discussion_r108254598
--- Diff:
core/src/test/scala/org/apache/spark/storage/BlockManagerReplicationSuite.scala
---
@@ -481,27 +481,39 @@ class BlockManagerProactiveReplicationSuite extends
BlockManagerReplicationBehav
assert(blockLocations.size === replicationFactor)
// remove a random blockManager
- val executorsToRemove = blockLocations.take(replicationFactor - 1)
+ val executorsToRemove = blockLocations.take(replicationFactor -
1).toSet
logInfo(s"Removing $executorsToRemove")
- executorsToRemove.foreach{exec =>
- master.removeExecutor(exec.executorId)
+ initialStores.filter(bm =>
executorsToRemove.contains(bm.blockManagerId)).foreach { bm =>
+ master.removeExecutor(bm.blockManagerId.executorId)
+ bm.stop()
// giving enough time for replication to happen and new block be
reported to master
- Thread.sleep(200)
+ eventually(timeout(5 seconds), interval(100 millis)) {
+ val newLocations = master.getLocations(blockId).toSet
+ assert(newLocations.size === replicationFactor)
+ }
}
- val newLocations = eventually(timeout(5 seconds), interval(10 millis))
{
+ val newLocations = eventually(timeout(5 seconds), interval(100
millis)) {
val _newLocations = master.getLocations(blockId).toSet
assert(_newLocations.size === replicationFactor)
_newLocations
}
logInfo(s"New locations : $newLocations")
- // there should only be one common block manager between initial and
new locations
- assert(newLocations.intersect(blockLocations.toSet).size === 1)
- // check if all the read locks have been released
+ // new locations should not contain stopped block managers
+ assert(newLocations.forall(bmId => !executorsToRemove.contains(bmId)),
+ "New locations contain stopped block managers.")
+
+ // this is to ensure the last read lock gets released before we try to
+ // check for read-locks. The check for read-locks using the method
below is not
+ // idempotent, and therefore can't be used in an `eventually` block.
+ Thread.sleep(500)
--- End diff --
scala test has an eventually primitive that you can use for this; I
submitted a PR to your branch with what I was thinking here:
https://github.com/shubhamchopra/spark/pull/1/commits/0c2eb9360c027e5d8bc8381e7e1bd56d7911527a
In general we should avoid sleeping in tests -- because it tends to lead to
Jenkins failures when Jenkins is slow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
