[SPARK-25776][CORE]The disk write buffer size must be greater than 12 #22754

10110346 · 2018-10-17T07:11:36Z

What changes were proposed in this pull request?

In UnsafeSorterSpillWriter.java, when we write a record to a spill file wtih void write(Object baseObject, long baseOffset, int recordLength, long keyPrefix), recordLength and keyPrefix will be written the disk write buffer first, and these will take 12 bytes, so the disk write buffer size must be greater than 12.

If diskWriteBufferSize is 10, it will print this exception info:

java.lang.ArrayIndexOutOfBoundsException: 10
at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.writeLongToBuffer (UnsafeSorterSpillWriter.java:91)
at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.write(UnsafeSorterSpillWriter.java:123)
at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spillIterator(UnsafeExternalSorter.java:498)
at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill(UnsafeExternalSorter.java:222)
at org.apache.spark.memory.MemoryConsumer.spill(MemoryConsumer.java:65)

How was this patch tested?

Existing UT in UnsafeExternalSorterSuite

SparkQA · 2018-10-17T11:22:15Z

Test build #97484 has finished for PR 22754 at commit dfc4b65.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-10-18T11:31:41Z

Good catch. My suggestion is to create a JIRA entry.
One question: can we set 12 into this property?

10110346 · 2018-10-19T02:05:04Z

If we set 12 into this, freeSpaceInWriteBuffer will be 0, and the length of copyMemory will always be 0, so it is not allowed to set 12 into this property.
https://github.com/apache/spark/blob/dfc4b65088602ee45e0babe22e64e205fab3e82b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillWriter.java#L128

10110346 · 2018-10-19T02:06:12Z

@kiszk Thanks，I will create a JIRA.

kiszk · 2018-10-21T17:56:25Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillWriter.java

Can we refine this comment to explain more than 12 bytes are required?
For example, space used by prefix + len + recordLength is more than 4 + 8 bytes?

core/src/main/scala/org/apache/spark/internal/config/package.scala

kiszk · 2018-10-21T18:04:10Z

Thank you for your clarification.

10110346 · 2018-10-22T01:30:26Z

Thank you for your review, I will update it @kiszk

SparkQA · 2018-10-22T03:28:44Z

Test build #97689 has started for PR 22754 at commit a0d36c7.

SparkQA · 2018-10-22T07:05:04Z

Test build #97693 has finished for PR 22754 at commit a0d36c7.

This patch fails due to an unknown error code, -9.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-10-22T07:05:05Z

Test build #97696 has finished for PR 22754 at commit dfc4b65.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-22T12:13:06Z

Test build #97758 has finished for PR 22754 at commit a0d36c7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-22T12:20:15Z

Test build #97763 has finished for PR 22754 at commit dfc4b65.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-22T13:13:12Z

Test build #97776 has started for PR 22754 at commit dfc4b65.

SparkQA · 2018-10-22T13:13:14Z

Test build #97770 has started for PR 22754 at commit a0d36c7.

srowen · 2018-10-22T23:06:20Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillWriter.java

assert shouldn't be used to check arguments in public APIs. But, despite its visibility I'm not sure if this is really a public API.

I am not sure too， but I see many places(BitSetMethods.java, HeapMemoryAllocator.java, LongArray.java) that use it like this.

Doesn't the updated checkValue of spark.shuffle.spill.diskWriteBufferSize already guarantee this?

Yes, it can guarantee this.
Here explains why it must be greater than 12.

If just for explaining it, maybe you can put a comment on where diskWriteBufferSize is defined, instead of an assert.

Where is the best place of this comment? I am neutral on this.

SparkQA · 2018-10-23T04:49:18Z

Test build #97887 has finished for PR 22754 at commit 6f8404b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-23T07:05:01Z

Test build #97903 has finished for PR 22754 at commit 6f8404b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-10-23T09:53:03Z

You can remove [MINOR] from the title since there is a JIRA ticket now.

viirya · 2018-10-23T09:53:17Z

retest this please.

SparkQA · 2018-10-23T13:13:19Z

Test build #97916 has finished for PR 22754 at commit 6f8404b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-23T13:26:05Z

Test build #97917 has finished for PR 22754 at commit 6f8404b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

10110346

retest this please

SparkQA · 2018-10-24T00:41:26Z

Test build #97951 has finished for PR 22754 at commit 6f8404b.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-25T05:39:00Z

Test build #97996 has finished for PR 22754 at commit b2ca621.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-10-28T08:18:40Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillWriter.java

nit: For a multiple-line comment, a starting line /** does not have a text.

SparkQA · 2018-10-28T10:51:27Z

Test build #98157 has finished for PR 22754 at commit c97906c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-28T12:47:38Z

Test build #98165 has finished for PR 22754 at commit c883f4b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-11-04T02:20:01Z

LGTM, pending Jenkins

kiszk · 2018-11-04T02:20:23Z

retest this please

SparkQA · 2018-11-04T06:27:11Z

Test build #98434 has finished for PR 22754 at commit c883f4b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-11-04T17:04:44Z

Thanks! merging to master

## What changes were proposed in this pull request? In `UnsafeSorterSpillWriter.java`, when we write a record to a spill file wtih ` void write(Object baseObject, long baseOffset, int recordLength, long keyPrefix)`, `recordLength` and `keyPrefix` will be written the disk write buffer first, and these will take 12 bytes, so the disk write buffer size must be greater than 12. If `diskWriteBufferSize` is 10, it will print this exception info: _java.lang.ArrayIndexOutOfBoundsException: 10 at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.writeLongToBuffer (UnsafeSorterSpillWriter.java:91) at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.write(UnsafeSorterSpillWriter.java:123) at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spillIterator(UnsafeExternalSorter.java:498) at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill(UnsafeExternalSorter.java:222) at org.apache.spark.memory.MemoryConsumer.spill(MemoryConsumer.java:65)_ ## How was this patch tested? Existing UT in `UnsafeExternalSorterSuite` Closes apache#22754 from 10110346/diskWriteBufferSize. Authored-by: liuxian <[email protected]> Signed-off-by: Kazuaki Ishizaki <[email protected]>

dongjoon-hyun · 2019-04-26T22:25:17Z

Hi, @kiszk .
Can we have this in branch-2.4 too since Spark 2.4.x is the last version of 2.x line which we should support for a while?

…han 12 apache#22754 In UnsafeSorterSpillWriter.java, when we write a record to a spill file wtih void write(Object baseObject, long baseOffset, int recordLength, long keyPrefix), recordLength and keyPrefix will be written the disk write buffer first, and these will take 12 bytes, so the disk write buffer size must be greater than 12. If diskWriteBufferSize is 10, it will print this exception info: java.lang.ArrayIndexOutOfBoundsException: 10 at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.writeLongToBuffer (UnsafeSorterSpillWriter.java:91) at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.write(UnsafeSorterSpillWriter.java:123) at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spillIterator(UnsafeExternalSorter.java:498) at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill(UnsafeExternalSorter.java:222) at org.apache.spark.memory.MemoryConsumer.spill(MemoryConsumer.java:65)

Ref: LIHADOOP-42707 In `UnsafeSorterSpillWriter.java`, when we write a record to a spill file wtih ` void write(Object baseObject, long baseOffset, int recordLength, long keyPrefix)`, `recordLength` and `keyPrefix` will be written the disk write buffer first, and these will take 12 bytes, so the disk write buffer size must be greater than 12. If `diskWriteBufferSize` is 10, it will print this exception info: _java.lang.ArrayIndexOutOfBoundsException: 10 at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.writeLongToBuffer (UnsafeSorterSpillWriter.java:91) at org.apache.spark.util.collection.unsafe.sort.UnsafeSorterSpillWriter.write(UnsafeSorterSpillWriter.java:123) at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spillIterator(UnsafeExternalSorter.java:498) at org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill(UnsafeExternalSorter.java:222) at org.apache.spark.memory.MemoryConsumer.spill(MemoryConsumer.java:65)_ Existing UT in `UnsafeExternalSorterSuite` Closes apache#22754 from 10110346/diskWriteBufferSize. Authored-by: liuxian <[email protected]> Signed-off-by: Kazuaki Ishizaki <[email protected]> (cherry picked from commit 6c9e5ac) RB=1518191 BUG=LIHADOOP-42707 G=superfriends-reviewers R=fli,mshen,yezhou,edlu A=fli

10110346 changed the title ~~[CORE][MINOR]The disk write buffer size must be greater than 12~~ [MINOR][CORE]The disk write buffer size must be greater than 12 Oct 17, 2018

10110346 changed the title ~~[MINOR][CORE]The disk write buffer size must be greater than 12~~ [SPARK-25776][CORE][MINOR]The disk write buffer size must be greater than 12 Oct 19, 2018

kiszk reviewed Oct 21, 2018

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated Show resolved Hide resolved

10110346 force-pushed the diskWriteBufferSize branch 2 times, most recently from 59bf755 to a0d36c7 Compare October 22, 2018 02:05

srowen reviewed Oct 22, 2018

View reviewed changes

10110346 force-pushed the diskWriteBufferSize branch from a0d36c7 to 6f8404b Compare October 23, 2018 01:24

10110346 changed the title ~~[SPARK-25776][CORE][MINOR]The disk write buffer size must be greater than 12~~ [SPARK-25776][CORE]The disk write buffer size must be greater than 12 Oct 23, 2018

10110346 commented Oct 23, 2018

View reviewed changes

10110346 force-pushed the diskWriteBufferSize branch from b2ca621 to c97906c Compare October 28, 2018 07:24

kiszk reviewed Oct 28, 2018

View reviewed changes

The disk write buffer size must be greater than 12

c883f4b

10110346 force-pushed the diskWriteBufferSize branch from c97906c to c883f4b Compare October 28, 2018 08:48

asfgit closed this in 6c9e5ac Nov 4, 2018

[SPARK-25776][CORE]The disk write buffer size must be greater than 12 #22754

[SPARK-25776][CORE]The disk write buffer size must be greater than 12 #22754

Uh oh!

Conversation

10110346 commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Oct 17, 2018

Uh oh!

kiszk commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

10110346 commented Oct 19, 2018

Uh oh!

10110346 commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kiszk commented Oct 21, 2018

Uh oh!

10110346 commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 23, 2018

Uh oh!

SparkQA commented Oct 23, 2018

Uh oh!

viirya commented Oct 23, 2018

Uh oh!

viirya commented Oct 23, 2018

Uh oh!

SparkQA commented Oct 23, 2018

Uh oh!

SparkQA commented Oct 23, 2018

Uh oh!

10110346 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 24, 2018

Uh oh!

SparkQA commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 28, 2018

Uh oh!

SparkQA commented Oct 28, 2018

10110346 commented Oct 17, 2018 •

edited

Loading

kiszk commented Oct 18, 2018 •

edited

Loading

viirya Oct 23, 2018 •

edited

Loading

10110346 left a comment •

edited

Loading

kiszk commented Nov 4, 2018 •

edited

Loading