Skip to content

[SPARK-26507][CORE] Fix core tests for Java 11 #23419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
srowen wants to merge 3 commits into from
Closed

[SPARK-26507][CORE] Fix core tests for Java 11 #23419

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4b0652
Additional Java 11 test fixes
srowen Dec 30, 2018
729a3c9
See if TimSort passes with 4g heap
srowen Jan 1, 2019
e4551bd
Revert mem change to 3g
srowen Jan 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions core/src/main/scala/org/apache/spark/util/Utils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2740,13 +2740,17 @@ private[spark] object Utils extends Logging {

/**
* Safer than Class obj's getSimpleName which may throw Malformed class name error in scala.
* This method mimicks scalatest's getSimpleNameOfAnObjectsClass.
* This method mimics scalatest's getSimpleNameOfAnObjectsClass.
*/
def getSimpleName(cls: Class[_]): String = {
try {
return cls.getSimpleName
cls.getSimpleName
} catch {
case err: InternalError => return stripDollars(stripPackages(cls.getName))
// TODO: the value returned here isn't even quite right; it returns simple names
// like UtilsSuite$MalformedClassObject$MalformedClass instead of MalformedClass
// The exact value may not matter much as it's used in log statements
case _: InternalError =>
stripDollars(stripPackages(cls.getName))
}
}

Expand Down
9 changes: 4 additions & 5 deletions core/src/test/scala/org/apache/spark/metrics/source/AccumulatorSourceSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@

package org.apache.spark.metrics.source

import com.codahale.metrics.MetricRegistry
import org.mockito.ArgumentCaptor
import org.mockito.Mockito.{mock, never, spy, times, verify, when}
import org.mockito.Mockito.{mock, times, verify, when}

import org.apache.spark.{SparkContext, SparkEnv, SparkFunSuite}
import org.apache.spark.metrics.MetricsSystem
Expand All @@ -37,7 +36,7 @@ class AccumulatorSourceSuite extends SparkFunSuite {
val accs = Map("my-accumulator-1" -> acc1,
"my-accumulator-2" -> acc2)
LongAccumulatorSource.register(mockContext, accs)
val captor = new ArgumentCaptor[AccumulatorSource]()
val captor = ArgumentCaptor.forClass(classOf[AccumulatorSource])

@srowen srowen Dec 31, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually just cleaning up a deprecation warning along the way; not strictly required for Java 11

verify(mockMetricSystem, times(1)).registerSource(captor.capture())
val source = captor.getValue()
val gauges = source.metricRegistry.getGauges()
Expand All @@ -59,7 +58,7 @@ class AccumulatorSourceSuite extends SparkFunSuite {
val accs = Map("my-accumulator-1" -> acc1,
"my-accumulator-2" -> acc2)
LongAccumulatorSource.register(mockContext, accs)
val captor = new ArgumentCaptor[AccumulatorSource]()
val captor = ArgumentCaptor.forClass(classOf[AccumulatorSource])
verify(mockMetricSystem, times(1)).registerSource(captor.capture())
val source = captor.getValue()
val gauges = source.metricRegistry.getGauges()
Expand All @@ -81,7 +80,7 @@ class AccumulatorSourceSuite extends SparkFunSuite {
"my-accumulator-1" -> acc1,
"my-accumulator-2" -> acc2)
DoubleAccumulatorSource.register(mockContext, accs)
val captor = new ArgumentCaptor[AccumulatorSource]()
val captor = ArgumentCaptor.forClass(classOf[AccumulatorSource])
verify(mockMetricSystem, times(1)).registerSource(captor.capture())
val source = captor.getValue()
val gauges = source.metricRegistry.getGauges()
Expand Down
16 changes: 11 additions & 5 deletions core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,13 +761,13 @@ private[spark] object JsonProtocolSuite extends Assertions {
}

private def assertJsonStringEquals(expected: String, actual: String, metadata: String) {
val expectedJson = pretty(parse(expected))
val actualJson = pretty(parse(actual))
val expectedJson = parse(expected)
val actualJson = parse(actual)
if (expectedJson != actualJson) {
// scalastyle:off
// This prints something useful if the JSON strings don't match
println("=== EXPECTED ===\n" + expectedJson + "\n")
println("=== ACTUAL ===\n" + actualJson + "\n")
println(s"=== EXPECTED ===\n${pretty(expectedJson)}\n")

@srowen srowen Dec 31, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of Properties, based on Hashtable, is returning elements in a different order in Java 11. Just comparing the actual JSON content rather than its string representation shows it still produces semantically correct output, so I changed the test.

println(s"=== ACTUAL ===\n${pretty(actualJson)}\n")
// scalastyle:on
throw new TestFailedException(s"$metadata JSON did not equal", 1)
}
Expand Down Expand Up @@ -807,7 +807,13 @@ private[spark] object JsonProtocolSuite extends Assertions {
}

private def assertStackTraceElementEquals(ste1: StackTraceElement, ste2: StackTraceElement) {
assert(ste1 === ste2)
// This mimics the equals() method from Java 8 and earlier. Java 9 adds checks for
// class loader and module, which will cause them to be not equal, when we don't
// care about those
assert(ste1.getClassName === ste2.getClassName)
assert(ste1.getMethodName === ste2.getMethodName)
assert(ste1.getLineNumber === ste2.getLineNumber)
assert(ste1.getFileName === ste2.getFileName)
}

/** ----------------------------------- *
Expand Down
16 changes: 0 additions & 16 deletions core/src/test/scala/org/apache/spark/util/UtilsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,22 +1156,6 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}

object MalformedClassObject {
class MalformedClass
}

test("Safe getSimpleName") {
// getSimpleName on class of MalformedClass will result in error: Malformed class name
// Utils.getSimpleName works
val err = intercept[java.lang.InternalError] {
classOf[MalformedClassObject.MalformedClass].getSimpleName
}
assert(err.getMessage === "Malformed class name")

assert(Utils.getSimpleName(classOf[MalformedClassObject.MalformedClass]) ===
"UtilsSuite$MalformedClassObject$MalformedClass")
}

test("stringHalfWidth") {
// scalastyle:off nonascii
assert(Utils.stringHalfWidth(null) == 0)
Expand Down
2 changes: 1 addition & 1 deletion launcher/src/test/java/org/apache/spark/launcher/SparkSubmitCommandBuilderSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public void testPySparkLauncher() throws Exception {

Map<String, String> env = new HashMap<>();
List<String> cmd = buildCommand(sparkSubmitArgs, env);
assertEquals("python", cmd.get(cmd.size() - 1));
assertTrue(Arrays.asList("python", "python2", "python3").contains(cmd.get(cmd.size() - 1)));

@srowen srowen Dec 31, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not strictly related, but something I caught while debugging. The Pyspark python interpreter might legitimately be set to a few other values.

assertEquals(
String.format("\"%s\" \"foo\" \"%s\" \"bar\" \"%s\"",
parser.MASTER, parser.DEPLOY_MODE, SparkSubmitCommandBuilder.PYSPARK_SHELL_RESOURCE),
Expand Down
6 changes: 4 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2060,6 +2060,8 @@
</executions>
<configuration>
<scalaVersion>${scala.version}</scalaVersion>
<checkMultipleScalaVersions>true</checkMultipleScalaVersions>

@srowen srowen Dec 31, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not strictly related, but helpful when I was debugging another Java 11 issue.

<failOnMultipleScalaVersions>true</failOnMultipleScalaVersions>
<recompileMode>incremental</recompileMode>
<useZincServer>true</useZincServer>
<args>
Expand Down Expand Up @@ -2113,7 +2115,7 @@
<include>**/*Suite.java</include>
</includes>
<reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
<argLine>-ea -Xmx3g -Xss4m -XX:ReservedCodeCacheSize=${CodeCacheSize}</argLine>
<argLine>-ea -Xmx6g -Xss4m -XX:ReservedCodeCacheSize=${CodeCacheSize}</argLine>

@srowen srowen Dec 31, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required now to make TimSort tests pass in Java 11, probably because of how direct buffer handling has changed.

@hvanhovell hvanhovell Dec 31, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to know what changed. Do you have an idea?

@kiszk kiszk Dec 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What tests failed?

@srowen srowen Dec 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TimSort test failed, where it allocates a very very large array.
I'm pretty sure it's because we can't get around the off-heap allocation limits in Java 9+ (see https://github.com/apache/spark/pull/22993/files). I think this would also pass if we manually set the off-heap limit very high, or allowed access to the Cleaner class on the command line. Upping the memory seemed like a simpler workaround but those should be fine too.

EDIT: I don't think this is right. See #23419 (comment)

@felixcheung felixcheung Jan 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the % changed is a quite a bit though

@dongjoon-hyun dongjoon-hyun Jan 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for increasing this. I also saw multiple TimSort OOM failures even in Apache Spark Jenkins environment(JDK8).

@srowen srowen Jan 1, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixcheung Let me try 4G or 5G. I think I just doubled it and it worked so I left it, but may not be necessary to make it that big.

<environmentVariables>
<!--
Setting SPARK_DIST_CLASSPATH is a simple way to make sure any child processes
Expand Down Expand Up @@ -2163,7 +2165,7 @@
<reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
<junitxml>.</junitxml>
<filereports>SparkTestSuite.txt</filereports>
<argLine>-ea -Xmx3g -Xss4m -XX:ReservedCodeCacheSize=${CodeCacheSize}</argLine>
<argLine>-ea -Xmx6g -Xss4m -XX:ReservedCodeCacheSize=${CodeCacheSize}</argLine>
<stderr/>
<environmentVariables>
<!--
Expand Down