From c4b6920ed24fd6398da27b978380427e3b0cb62a Mon Sep 17 00:00:00 2001 From: Sam Davarnia <> Date: Sun, 30 Sep 2018 07:36:33 -0700 Subject: [PATCH 1/4] fix liniting errors --- .../collection/unsafe/sort/UnsafeInMemorySorter.java | 9 ++++++--- .../unsafe/sort/UnsafeExternalSorterSuite.java | 9 ++++++--- .../spark/sql/execution/UnsafeKVExternalSorter.java | 2 +- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java index 839b41db4082b..4975e7087d005 100644 --- a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java +++ b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java @@ -174,9 +174,12 @@ public void reset() { if (consumer != null) { consumer.freeArray(array); // the call to consumer.allocateArray may trigger a spill - // which in turn access this instance and eventually re-enter this method and try to free the array again. - // by setting the array to null and its length to 0 we effectively make the spill code-path a no-op. - // setting the array to null also indicates that it has already been de-allocated which prevents a double de-allocation in free(). + // which in turn access this instance and eventually re-enter this method + // and try to free the array again. + // by setting the array to null and its length to 0 + // we effectively make the spill code-path a no-op. + // setting the array to null also indicates that it has already been + // de-allocated which prevents a double de-allocation in free(). array = null; usableCapacity = 0; pos = 0; diff --git a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java index cce01a3275421..7d3b4ddbc4aa8 100644 --- a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java +++ b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java @@ -470,17 +470,20 @@ public void testOOMDuringSpill() throws Exception { insertNumber(sorter, i); } // we expect the next insert to attempt growing the pointerssArray - // first allocation is expected to fail, then a spill is triggered which attempts another allocation + // first allocation is expected to fail, then a spill is + // triggered which attempts another allocation // which also fails and we expect to see this OOM here. // the original code messed with a released array within the spill code // and ended up with a failed assertion. - // we also expect the location of the OOM to be org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset + // we also expect the location of the OOM to be + // org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset memoryManager.markconsequentOOM(2); try { insertNumber(sorter, 1024); fail("expected OutOfMmoryError but it seems operation surprisingly succeeded"); } - // we expect an OutOfMemoryError here, anything else (i.e the original NPE is a failure) + // we expect an OutOfMemoryError here, anything else + // (i.e the original NPE is a failure) catch (OutOfMemoryError oom){ String oomStackTrace = Utils.exceptionString(oom); assertThat("expected OutOfMemoryError in org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset", diff --git a/sql/core/src/main/java/org/apache/spark/sql/execution/UnsafeKVExternalSorter.java b/sql/core/src/main/java/org/apache/spark/sql/execution/UnsafeKVExternalSorter.java index 7549decf57488..44bc681f0d11f 100644 --- a/sql/core/src/main/java/org/apache/spark/sql/execution/UnsafeKVExternalSorter.java +++ b/sql/core/src/main/java/org/apache/spark/sql/execution/UnsafeKVExternalSorter.java @@ -259,7 +259,7 @@ public int compare( Object baseObj2, long baseOff2, int baseLen2) { - // Note that since ordering doesn't need the total length of the record, we just pass -1 + // Note that since ordering doesn't need the total length of the record, we just pass -1 // into the row. row1.pointTo(baseObj1, baseOff1 + 4, -1); row2.pointTo(baseObj2, baseOff2 + 4, -1); From 009d4cf0529917de32483f9d7686b7dcdbe6e400 Mon Sep 17 00:00:00 2001 From: Sam Davarnia Date: Mon, 1 Oct 2018 10:26:04 -0700 Subject: [PATCH 2/4] pr comments --- .../util/collection/unsafe/sort/UnsafeInMemorySorter.java | 4 ++-- .../collection/unsafe/sort/UnsafeExternalSorterSuite.java | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java index 4975e7087d005..8a034f263d518 100644 --- a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java +++ b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java @@ -176,9 +176,9 @@ public void reset() { // the call to consumer.allocateArray may trigger a spill // which in turn access this instance and eventually re-enter this method // and try to free the array again. - // by setting the array to null and its length to 0 + // By setting the array to null and its length to 0 // we effectively make the spill code-path a no-op. - // setting the array to null also indicates that it has already been + // Setting the array to null also indicates that it has already been // de-allocated which prevents a double de-allocation in free(). array = null; usableCapacity = 0; diff --git a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java index 7d3b4ddbc4aa8..050c570246646 100644 --- a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java +++ b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java @@ -460,7 +460,7 @@ public void testPeakMemoryUsed() throws Exception { @Test public void testOOMDuringSpill() throws Exception { final UnsafeExternalSorter sorter = newSorter(); - // we assume that given default configuration, + // We assume that given default configuration, // the size of the data we insert to the sorter (ints) // and assuming we shouldn't spill before pointers array is exhausted // (memory manager is not configured to throw at this point) @@ -469,20 +469,20 @@ public void testOOMDuringSpill() throws Exception { for (int i = 0; sorter.hasSpaceForAnotherRecord(); ++i) { insertNumber(sorter, i); } - // we expect the next insert to attempt growing the pointerssArray + // We expect the next insert to attempt growing the pointerssArray // first allocation is expected to fail, then a spill is // triggered which attempts another allocation // which also fails and we expect to see this OOM here. // the original code messed with a released array within the spill code // and ended up with a failed assertion. - // we also expect the location of the OOM to be + // We also expect the location of the OOM to be // org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset memoryManager.markconsequentOOM(2); try { insertNumber(sorter, 1024); fail("expected OutOfMmoryError but it seems operation surprisingly succeeded"); } - // we expect an OutOfMemoryError here, anything else + // We expect an OutOfMemoryError here, anything else // (i.e the original NPE is a failure) catch (OutOfMemoryError oom){ String oomStackTrace = Utils.exceptionString(oom); From 2e8836ba0cab70243a8b5ef8845d045d65856aa1 Mon Sep 17 00:00:00 2001 From: Sam Davarnia <> Date: Mon, 1 Oct 2018 19:06:47 -0700 Subject: [PATCH 3/4] lint fixes --- .../spark/unsafe/types/UTF8StringSuite.java | 1 - .../unsafe/sort/UnsafeInMemorySorter.java | 6 +++--- .../unsafe/sort/UnsafeExternalSorterSuite.java | 15 +++++++++------ .../unsafe/sort/UnsafeInMemorySorterSuite.java | 9 ++++++--- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java b/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java index 33b9e1139baf5..773461bcc9c38 100644 --- a/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java +++ b/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java @@ -63,7 +63,6 @@ public void basicTest() { checkBasic("hello", 5); // 5 * 1 byte chars checkBasic("大 千 世 界", 7); checkBasic("︽﹋％", 3); // 3 * 3 bytes chars - checkBasic("\uD83E\uDD19", 1); // 4 bytes char } @Test diff --git a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java index 8a034f263d518..b02581199295e 100644 --- a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java +++ b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java @@ -174,11 +174,11 @@ public void reset() { if (consumer != null) { consumer.freeArray(array); // the call to consumer.allocateArray may trigger a spill - // which in turn access this instance and eventually re-enter this method + // which in turn access this instance and eventually re-enter this method // and try to free the array again. - // By setting the array to null and its length to 0 + // By setting the array to null and its length to 0 // we effectively make the spill code-path a no-op. - // Setting the array to null also indicates that it has already been + // Setting the array to null also indicates that it has already been // de-allocated which prevents a double de-allocation in free(). array = null; usableCapacity = 0; diff --git a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java index 050c570246646..12497c7f92ed2 100644 --- a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java +++ b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java @@ -470,25 +470,28 @@ public void testOOMDuringSpill() throws Exception { insertNumber(sorter, i); } // We expect the next insert to attempt growing the pointerssArray - // first allocation is expected to fail, then a spill is + // first allocation is expected to fail, then a spill is // triggered which attempts another allocation // which also fails and we expect to see this OOM here. // the original code messed with a released array within the spill code // and ended up with a failed assertion. - // We also expect the location of the OOM to be + // We also expect the location of the OOM to be // org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset memoryManager.markconsequentOOM(2); try { insertNumber(sorter, 1024); - fail("expected OutOfMmoryError but it seems operation surprisingly succeeded"); + fail("expected OutOfMmoryError but " + + "it seems operation surprisingly succeeded"); } - // We expect an OutOfMemoryError here, anything else + // We expect an OutOfMemoryError here, anything else // (i.e the original NPE is a failure) catch (OutOfMemoryError oom){ String oomStackTrace = Utils.exceptionString(oom); - assertThat("expected OutOfMemoryError in org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset", + assertThat("expected OutOfMemoryError in " + + "org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset", oomStackTrace, - Matchers.containsString("org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset")); + Matchers.containsString("org.apache.spark.util.collection." + + "unsafe.sort.UnsafeInMemorySorter.reset")); } } } diff --git a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java index cfb00307fa883..359426d829322 100644 --- a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java +++ b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java @@ -174,16 +174,19 @@ public int compare( } }; UnsafeInMemorySorter sorter = new UnsafeInMemorySorter(consumer, memoryManager, - recordComparator, prefixComparator, 100, shouldUseRadixSort()); + recordComparator, prefixComparator, + 100, shouldUseRadixSort()); testMemoryManager.markExecutionAsOutOfMemoryOnce(); try { sorter.reset(); - fail("expected OutOfMmoryError but it seems operation surprisingly succeeded"); + fail("expected OutOfMmoryError " + + "but it seems operation surprisingly succeeded"); } catch (OutOfMemoryError oom) { // as expected } - // [SPARK-21907] this failed on NPE at org.apache.spark.memory.MemoryConsumer.freeArray(MemoryConsumer.java:108) + // [SPARK-21907] this failed on NPE at + // org.apache.spark.memory.MemoryConsumer.freeArray(MemoryConsumer.java:108) sorter.free(); // simulate a 'back to back' free. sorter.free(); From ff619cdd23cb0a86ce0acb972c0dbb40335c2437 Mon Sep 17 00:00:00 2001 From: Dongjoon Hyun Date: Mon, 1 Oct 2018 23:18:29 -0700 Subject: [PATCH 4/4] fix --- .../apache/spark/unsafe/types/UTF8StringSuite.java | 3 +++ .../unsafe/sort/UnsafeExternalSorterSuite.java | 13 ++++++------- .../unsafe/sort/UnsafeInMemorySorterSuite.java | 6 ++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java b/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java index 773461bcc9c38..ea79c28151eda 100644 --- a/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java +++ b/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java @@ -63,6 +63,9 @@ public void basicTest() { checkBasic("hello", 5); // 5 * 1 byte chars checkBasic("大 千 世 界", 7); checkBasic("︽﹋％", 3); // 3 * 3 bytes chars + // checkstyle.off: AvoidEscapedUnicodeCharacters + checkBasic("\uD83E\uDD19", 1); // 4 bytes char + // checkstyle.on: AvoidEscapedUnicodeCharacters } @Test diff --git a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java index 12497c7f92ed2..17c4d7e5b9883 100644 --- a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java +++ b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java @@ -480,18 +480,17 @@ public void testOOMDuringSpill() throws Exception { memoryManager.markconsequentOOM(2); try { insertNumber(sorter, 1024); - fail("expected OutOfMmoryError but " + - "it seems operation surprisingly succeeded"); + fail("expected OutOfMmoryError but it seems operation surprisingly succeeded"); } // We expect an OutOfMemoryError here, anything else // (i.e the original NPE is a failure) catch (OutOfMemoryError oom){ String oomStackTrace = Utils.exceptionString(oom); - assertThat("expected OutOfMemoryError in " + - "org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset", - oomStackTrace, - Matchers.containsString("org.apache.spark.util.collection." + - "unsafe.sort.UnsafeInMemorySorter.reset")); + assertThat("expected OutOfMemoryError in " + + "org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset", + oomStackTrace, + Matchers.containsString( + "org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset")); } } } diff --git a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java index 359426d829322..c145532328514 100644 --- a/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java +++ b/core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java @@ -174,14 +174,12 @@ public int compare( } }; UnsafeInMemorySorter sorter = new UnsafeInMemorySorter(consumer, memoryManager, - recordComparator, prefixComparator, - 100, shouldUseRadixSort()); + recordComparator, prefixComparator, 100, shouldUseRadixSort()); testMemoryManager.markExecutionAsOutOfMemoryOnce(); try { sorter.reset(); - fail("expected OutOfMmoryError " + - "but it seems operation surprisingly succeeded"); + fail("expected OutOfMmoryError but it seems operation surprisingly succeeded"); } catch (OutOfMemoryError oom) { // as expected }