[SPARK-44784][CONNECT] Make SBT testing hermetic. #42591

hvanhovell · 2023-08-21T20:50:54Z

What changes were proposed in this pull request?

This PR makes a bunch of changes to connect testing for the scala client:

We do not start the connect server with the SPARK_DIST_CLASSPATH environment variable. This is set by the build system, but its value for SBT and Maven is different. For SBT it also contained the client code.
We use dependency upload to add the dependencies needed for the tests. Currently this entails: the compiled test classes (class files), scalatest jars, and scalactic jars.
The use of classfile sync unearthed an issue with stubbing and the ExecutorClassLoader. If they load classes in the same namespace then stubbing will generate stubs for classes that can be loaded by the ExecutorClassLoader. Since this is mostly a testing issue I decided to move the test code to a different namespace. We should definitely fix this later on.
A bunch of tiny fixes.

Why are the changes needed?

SBT testing for connect leaked client side code into the server. This is a problem because tests pass and we sign-off on features that do not work when well in a normal environment. Stubbing was an example of this. Maven did not have this problem and was therefore more correct.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

It are mostly tests.

Was this patch authored or co-authored using generative AI tooling?

No. I write my own code thank you...

hvanhovell · 2023-08-21T20:54:31Z

...connect/client/jvm/src/test/scala/org/apache/spark/sql/UserDefinedFunctionE2ETestSuite.scala

      f.foreach(v => sum.addAndGet(v))
-      // The value should be 45
-      assert(sum.get() == -1)
+      throw new Exception("Success, processed records: " + sum.get())


This is a fun one. This was throwing a scalatest specific exception. The problem with this is that this error is decoded by the ResultHandler in Spark Core, this does not have the thread so things fail with a ClassNotFoundException instead of the expected exception.

hvanhovell · 2023-08-21T20:56:33Z

...erver/src/main/scala/org/apache/spark/sql/connect/artifact/SparkConnectArtifactManager.scala

-        Utils.getContextOrSparkClassLoader)
+    val prefixes = SparkEnv.get.conf.get(CONNECT_SCALA_UDF_STUB_PREFIXES)
+    val userClasspathFirst = SparkEnv.get.conf.get(EXECUTOR_USER_CLASS_PATH_FIRST)
+    val loader = if (prefixes.nonEmpty) {


For the record, this fixes nothing. It just makes sure we respect the userClasspathFirst setting.

hvanhovell · 2023-08-21T20:57:40Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/test/RemoteSparkSession.scala

+    val builder = new ProcessBuilder(command.result(): _*)
+    builder.directory(new File(sparkHome))
+    val environment = builder.environment()
+    environment.remove("SPARK_DIST_CLASSPATH")


This is the important bit, here we nuke the SPARK_DIST_CLASSPATH env variable.

hvanhovell · 2023-08-21T20:58:49Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/StubbingTestSuite.scala

+class StubbingTestSuite extends RemoteSparkSession {
+  private def eval[T](f: => T): T = f
+
+  test("capture of to-be stubbed class") {


This was a working reproduction for issue. Now that SparkResult has moved to common, I guess we need to find a new problem...

Yeah, this test case can't reproduce the problem. The new test requires a class like SparkResult

I manually revert the changes from SPARK-44806 based on this PR and verified that the test cases in SparkSessionE2ESuite can pass through Maven. However, it would be best if we could find a new target case.

I revamped the test.

I can confirm it is working with Maven, and that it triggers stubbing on the server side.

hvanhovell · 2023-08-22T02:52:30Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/test/RemoteSparkSession.scala

+
+    val command = Seq.newBuilder[String]
+    command += "bin/spark-submit"
+    command += "--driver-class-path" += connectJar


This is needed to make the test work with maven. Assembly might produce slightly different results.

hvanhovell · 2023-08-22T02:53:59Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/test/RemoteSparkSession.scala

+          .build())
+      .create()
+
+    // Execute an RPC which will get retried until the server is up.


Retries do not seem to work well for artifact upload, so I am falling back to this one.

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/test/RemoteSparkSession.scala

LuciferYang · 2023-08-23T08:23:43Z

A Java code style issue needs to be fixed.

src/test/java/org/apache/spark/sql/JavaEncoderSuite.java:[31,8] (imports) UnusedImports: Unused import - org.apache.spark.sql.connect.client.SparkConnectClient.

hvanhovell · 2023-08-25T15:08:39Z

@LuciferYang PTAL

LuciferYang

LGTM

LuciferYang · 2023-08-25T16:22:58Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/test/RemoteSparkSession.scala

+      .getProperty("java.class.path")
+      .split(File.pathSeparatorChar)
+      .filter(_.endsWith(".jar"))
+      .filter(e => e.contains("scalatest") || e.contains("scalactic"))


Nit:

/Users/yangjie01/.m2/repository/org/scalatestplus/scalacheck-1-17_2.12/3.2.16.0/scalacheck-1-17_2.12-3.2.16.0.jar /Users/yangjie01/.m2/repository/org/scalatestplus/mockito-4-11_2.12/3.2.16.0/mockito-4-11_2.12-3.2.16.0.jar /Users/yangjie01/.m2/repository/org/scalatestplus/selenium-4-9_2.12/3.2.16.0/selenium-4-9_2.12-3.2.16.0.jar

jars also include scalacheck, mockito and selenium due to the full path includes scalatestplus, is this expected?

LOL - I can make it more restrictive.

LuciferYang

LGTM

### What changes were proposed in this pull request? This PR makes a bunch of changes to connect testing for the scala client: - We do not start the connect server with the `SPARK_DIST_CLASSPATH ` environment variable. This is set by the build system, but its value for SBT and Maven is different. For SBT it also contained the client code. - We use dependency upload to add the dependencies needed for the tests. Currently this entails: the compiled test classes (class files), scalatest jars, and scalactic jars. - The use of classfile sync unearthed an issue with stubbing and the `ExecutorClassLoader`. If they load classes in the same namespace then stubbing will generate stubs for classes that can be loaded by the `ExecutorClassLoader`. Since this is mostly a testing issue I decided to move the test code to a different namespace. We should definitely fix this later on. - A bunch of tiny fixes. ### Why are the changes needed? SBT testing for connect leaked client side code into the server. This is a problem because tests pass and we sign-off on features that do not work when well in a normal environment. Stubbing was an example of this. Maven did not have this problem and was therefore more correct. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? It are mostly tests. ### Was this patch authored or co-authored using generative AI tooling? No. I write my own code thank you... Closes #42591 from hvanhovell/investigate-stubbing. Authored-by: Herman van Hovell <[email protected]> Signed-off-by: yangjie01 <[email protected]> (cherry picked from commit 9326615) Signed-off-by: yangjie01 <[email protected]>

LuciferYang · 2023-08-27T03:29:52Z

GA passed. Merged into master and 3.5. Thanks @hvanhovell

hvanhovell added 5 commits August 18, 2023 16:11

Improve dev scripts

950e579

Auto Sync Attempt

7ec722e

Big change

be2d133

Self Review

708704c

Merge remote-tracking branch 'apache/master' into investigate-stubbing

d047f75

github-actions bot added SQL STRUCTURED STREAMING BUILD CONNECT labels Aug 21, 2023

hvanhovell commented Aug 21, 2023

View reviewed changes

Fix maven build. Use internal retry mechanism.

566c1a2

hvanhovell commented Aug 22, 2023

View reviewed changes

hvanhovell requested a review from LuciferYang August 22, 2023 03:19

LuciferYang reviewed Aug 22, 2023

View reviewed changes

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/test/RemoteSparkSession.scala Outdated Show resolved Hide resolved

LuciferYang reviewed Aug 23, 2023

View reviewed changes

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/test/RemoteSparkSession.scala Outdated Show resolved Hide resolved

hvanhovell added 2 commits August 25, 2023 16:29

Merge remote-tracking branch 'apache/master' into investigate-stubbing

ca00b0a

Code Review, tests, and style

fca40ab

github-actions bot added the CORE label Aug 25, 2023

LuciferYang approved these changes Aug 25, 2023

View reviewed changes

hvanhovell added 2 commits August 26, 2023 04:37

Merge remote-tracking branch 'apache/master' into investigate-stubbing

cf42652

Reduce used dependencies.

c3b9893

LuciferYang approved these changes Aug 26, 2023

View reviewed changes

hvanhovell added 2 commits August 26, 2023 21:02

Merge remote-tracking branch 'apache/master' into investigate-stubbing

4e3380b

Style

b4ff731

LuciferYang approved these changes Aug 27, 2023

View reviewed changes

LuciferYang closed this in 9326615 Aug 27, 2023

[SPARK-44784][CONNECT] Make SBT testing hermetic. #42591

[SPARK-44784][CONNECT] Make SBT testing hermetic. #42591

Uh oh!

Conversation

hvanhovell commented Aug 21, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell Aug 21, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Aug 22, 2023 • 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!

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!

Uh oh!

Uh oh!

LuciferYang commented Aug 23, 2023

Uh oh!

hvanhovell commented Aug 25, 2023

Uh oh!

LuciferYang left a comment

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!

LuciferYang left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Aug 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hvanhovell Aug 21, 2023 •

edited

Loading

LuciferYang Aug 22, 2023 •

edited

Loading