Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10170#discussion_r47016581
--- Diff:
core/src/test/scala/org/apache/spark/memory/StaticMemoryManagerSuite.scala ---
@@ -85,33 +82,33 @@ class StaticMemoryManagerSuite extends
MemoryManagerSuite {
val (mm, ms) = makeThings(Long.MaxValue, maxStorageMem)
assert(mm.storageMemoryUsed === 0L)
assert(mm.acquireStorageMemory(dummyBlock, 10L, evictedBlocks))
- // `ensureFreeSpace` should be called with the number of bytes
requested
- assertEnsureFreeSpaceCalled(ms, 10L)
+ assertEvictBlocksToFreeSpaceNotCalled(ms)
assert(mm.storageMemoryUsed === 10L)
+
assert(mm.acquireStorageMemory(dummyBlock, 100L, evictedBlocks))
- assertEnsureFreeSpaceCalled(ms, 100L)
+ assertEvictBlocksToFreeSpaceNotCalled(ms)
assert(mm.storageMemoryUsed === 110L)
// Acquire more than the max, not granted
assert(!mm.acquireStorageMemory(dummyBlock, maxStorageMem + 1L,
evictedBlocks))
- assertEnsureFreeSpaceCalled(ms, maxStorageMem + 1L)
+ assertEvictBlocksToFreeSpaceNotCalled(ms)
assert(mm.storageMemoryUsed === 110L)
// Acquire up to the max, requests after this are still granted due to
LRU eviction
assert(mm.acquireStorageMemory(dummyBlock, maxStorageMem,
evictedBlocks))
- assertEnsureFreeSpaceCalled(ms, 1000L)
+ assertEvictBlocksToFreeSpaceCalled(ms, 110L)
assert(mm.storageMemoryUsed === 1000L)
assert(mm.acquireStorageMemory(dummyBlock, 1L, evictedBlocks))
- assertEnsureFreeSpaceCalled(ms, 1L)
+ assertEvictBlocksToFreeSpaceCalled(ms, 1L)
assert(mm.storageMemoryUsed === 1000L)
--- End diff --
It took me a while to understand why this one is still `1000L`. The
situation is as follows:
1. Put a 1000 bytes block
2. Put a 1 byte block, this should have evicted the first block
3. Storage memory used is still 1000 bytes somehow (I expected 1 byte)
Because of the way the test is set up, however, as of step (2) we actually
treat the 1000-byte block inserted in step (1) as 1000 1-byte blocks. When we
call `evictBlocksToFreeSpace` here we actually evicted a 1-byte block to put
another 1-byte block, so the storage memory usage remains at `1000L`.
Since this deviates from actual block manager behavior I think this line
warrants a two-line comment, maybe something like:
```
// Note: We evicted 1 byte to put another 1-byte block in, so the storage
memory used remains
// at 1000 bytes. This is different from real behavior, where the 1-byte
block would have evicted
// the 1000-byte block entirely. This is set up differently so we can write
finer-grained tests.
```
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
