Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/19311#discussion_r140815961
--- Diff:
core/src/test/scala/org/apache/spark/storage/MemoryStoreSuite.scala ---
@@ -407,4 +407,119 @@ class MemoryStoreSuite
})
assert(memoryStore.getSize(blockId) === 10000)
}
+
+ test("SPARK-22083: Release all locks in evictBlocksToFreeSpace") {
+ // Setup a memory store with many blocks cached, and then one request
which leads to multiple
+ // blocks getting evicted. We'll make the eviction throw an
exception, and make sure that
+ // all locks are released.
+ val ct = implicitly[ClassTag[Array[Byte]]]
+ def testFailureOnNthDrop(failAfterDroppingNBlocks: Int,
readLockAfterDrop: Boolean): Unit = {
--- End diff --
I don't think `validBlock` captures the intent here -- I don't see anything
valid or invalid about it either way. The part of the behavior which changes
is whether or not another thread grabs a reader lock on the thread after it
gets dropped to disk.
(To go along with that, we drop the block to disk, rather than just
evicting it completely, as otherwise there is nothing to grab a lock of. I
could always drop the block to disk, instead of having that depend on this, it
just seemed like another useful thing to check, whether the number of blocks
was successfully updated in `blockInfoManager`, when the block was dropped
completely.)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
