Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10170#discussion_r47035741
--- Diff:
core/src/test/scala/org/apache/spark/memory/UnifiedMemoryManagerSuite.scala ---
@@ -143,7 +148,25 @@ class UnifiedMemoryManagerSuite extends
MemoryManagerSuite with PrivateMethodTes
assert(mm.acquireExecutionMemory(400L, taskAttemptId,
MemoryMode.ON_HEAP) === 300L)
assert(mm.executionMemoryUsed === 600L)
assert(mm.storageMemoryUsed === 400L)
- assertEnsureFreeSpaceNotCalled(ms)
+ assertEvictBlocksToFreeSpaceNotCalled(ms)
+ assert(evictedBlocks.isEmpty)
+ }
+
+ test("execution can evict storage blocks when storage memory is below
max mem (SPARK-12165)") {
+ val maxMemory = 1000L
+ val taskAttemptId = 0L
+ val (mm, ms) = makeThings(maxMemory)
+ // Acquire enough storage memory to exceed the storage region size
+ assert(mm.acquireStorageMemory(dummyBlock, 750L, evictedBlocks))
+ assertEvictBlocksToFreeSpaceNotCalled(ms)
+ assert(mm.executionMemoryUsed === 0L)
+ assert(mm.storageMemoryUsed === 750L)
+ // Should now be able to require up to 500 bytes of memory
+ assert(mm.acquireExecutionMemory(500L, taskAttemptId,
MemoryMode.ON_HEAP) === 500L)
--- End diff --
> The problem isn't that "storage memory is below than max memory". Even in
the existing test "eviction evicts storage", storage memory (750) is below the
max memory (1000).
It's true that this was a little inprecise, but I think that "execution
memory requests smaller than free memory should evict storage" is also slightly
inprecise because this essentially sounds like a synonym for "execution evicts
storage when there is not enough free memory." I guess a slightly more accurate
description of SPARK-12165 would be something like "execution evicts storage if
free memory is insufficient and free memory > 0", since not _all_ situations
where there is insufficient free memory will trigger the first part of
SPARK-12165. This precision doesn't really matter, though, so I'm happy to
adopt your test name if you find it to be clearer.
---
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
