[SPARK-11078] Ensure spilling tests actually spill

[andrewor14]

#9084 uncovered that many tests that test spilling don't actually spill. This is a follow-up patch to fix that to ensure our unit tests actually catch potential bugs in spilling. The size of this patch is inflated by the refactoring of ExternalSorterSuite, which had a lot of duplicate code and logic.

[SparkQA]

Test build #43747 has finished for PR 9124 at commit 1b7fa3d.

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

[SparkQA]

Test build #1904 has finished for PR 9124 at commit 1b7fa3d.

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

[cloud-fan]

nit:

def mergeCombiners(
    buffer1: ArrayBuffer[String],
    buffer2: ArrayBuffer[String]): ArrayBuffer[String]

[andrewor14]

oops I screwed that up

[cloud-fan]

LGTM over all

[andrewor14]

OK, merging.

[SparkQA]

Test build #43812 has finished for PR 9124 at commit 7590b77.

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

[srowen]

@andrewor14 looks like we still have some failures of the form...

- spilling with compression *** FAILED ***
  java.lang.Exception: Test failed with compression using codec org.apache.spark.io.LZ4CompressionCodec:

assertion failed: expected groupByKey to spill, but did not
  at scala.Predef$.assert(Predef.scala:170)
  at org.apache.spark.TestUtils$.assertSpilled(TestUtils.scala:170)
  at org.apache.spark.util.collection.ExternalAppendOnlyMapSuite.org$apache$spark$util$collection$ExternalAppendOnlyMapSuite$$testSimpleSpilling(ExternalAppendOnlyMapSuite.scala:253)
  at org.apache.spark.util.collection.ExternalAppendOnlyMapSuite$$anonfun$10$$anonfun$apply$mcV$sp$8.apply(ExternalAppendOnlyMapSuite.scala:218)
  at org.apache.spark.util.collection.ExternalAppendOnlyMapSuite$$anonfun$10$$anonfun$apply$mcV$sp$8.apply(ExternalAppendOnlyMapSuite.scala:216)
  at scala.collection.immutable.Stream.foreach(Stream.scala:594)
  ...

Do you think it's possible that somehow there's a race here in reporting the metrics? so that things are spilling but not reported to the listener by the time the number of spilled tasks is checked?

I could try putting in an eventually to let it check repeatedly for a short time? any better ideas, or reasons to think that's not it?

[andrewor14]

I see, that's possible. The right thing to do here is to add a wait of some sort in the listener to block until we have received the stage completed event. We've done this in some other test listeners as well. (I can't remember which ones off the top of my head).