Github user eyalfa commented on a diff in the pull request:
https://github.com/apache/spark/pull/19181#discussion_r142005517
--- Diff:
core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java
---
@@ -503,6 +511,39 @@ public void testGetIterator() throws Exception {
verifyIntIterator(sorter.getIterator(279), 279, 300);
}
+ @Test
+ public void testOOMDuringSpill() throws Exception {
+ final UnsafeExternalSorter sorter = newSorter();
+ for (int i = 0; sorter.hasSpaceForAnotherRecord(); ++i) {
+ insertNumber(sorter, i);
+ }
+ // todo: this might actually not be zero if pageSize is somehow
configured differently,
+ // so we actually have to compute the expected spill size according to
the configured page size
+ assertEquals(0, sorter.getSpillSize());
--- End diff --
well, its a tricky one...
as far as I can track the construction and logic of the relevant classes
here, and given that we're sorting Ints here, we're not supposed to spill
before expandPointersArray requests additional memory, it is however very
difficult and 'involved' to actually check this during test.
I can set the memoryManager to choke **before** the first loop, this way if
my assumption breaks and the sorter attempts to allocate memory we'd get an OOM
sooner than expected, effectively failing the test.
@juliuszsompolski , do you find this approach better? it'd still need a
comment describing the assumption about memory usage by the sorter.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
