[SPARK-42215][CONNECT] Simplify Scala Client IT tests #40274

zhenlineo · 2023-03-04T01:06:04Z

What changes were proposed in this pull request?

Make use of the new spark-connect script to make the Scala client test to not directly depends on any other modules.
The dependency is still there but hidden by the spark-connect script. When calling the script to start the server, the script performs a build/sbt package to ensure all the jars are build and can be found in the correct path.

After the change, we can use the following commands to run the Scala client tests:

build/mvn clean
build/mvn compile -pl connector/connect/client/jvm
build/mvn test -pl connector/connect/client/jvm

build/sbt clean
build/sbt "testOnly org.apache.spark.sql.ClientE2ETestSuite"

build/sbt clean
build/sbt "connect-client-jvm/test"

Scala 2.13

build/mvn clean
build/mvn -Pscala-2.13 compile -pl connector/connect/client/jvm -am -DskipTests
build/mvn -Pscala-2.13 test -pl connector/connect/client/jvm

// These commands failed with errors to find catalyst ArrowWriter. The error seems unrelated to this change.

build/sbt clean
build/sbt "testOnly org.apache.spark.sql.ClientE2ETestSuite" -Pscala-2.13

build/sbt clean
build/sbt "connect-client-jvm/test" -Pscala-2.13

After the change, the waiting time to run the E2ESuite is ~3min for a clean build. Then ~1min for subsequent runs. The test is slower only because we moved the build time from many commands to this single command. There is no limitations to add more tests as the delay is only caused by the shared server start time. Once the server is started, the tests run fast.

Why are the changes needed?

A single command for maven users to mvn clean install to run tests and build.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Build, Manual tests.

zhenlineo · 2023-03-04T01:06:35Z

@hvanhovell
cc @LuciferYang

zhenlineo · 2023-03-04T01:11:50Z

The full error (even with the clean master branch):

build/mvn clean
build/mvn -Pscala-2.13 compile -pl connector/connect/client/jvm -am -DskipTests
build/mvn -Pscala-2.13 test -pl connector/connect/client/jvm -> errored here

[ERROR] [Error] /Users/zhen.li/code/spark-sbt/connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/connect/client/util/ConvertToArrow.scala:28: object arrow is not a member of package org.apache.spark.sql.execution
[ERROR] [Error] /Users/zhen.li/code/spark-sbt/connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/connect/client/util/ConvertToArrow.scala:46: not found: type ArrowWriter
[ERROR] [Error] /Users/zhen.li/code/spark-sbt/connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/connect/client/util/ConvertToArrow.scala:46: not found: value ArrowWriter

LuciferYang · 2023-03-04T02:01:30Z

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

@@ -76,7 +76,8 @@ class ClientE2ETestSuite extends RemoteSparkSession {
    assert(result(2) == 2)
  }

-  test("simple udf") {
+  // Ignore this test until the udf is fully implemented.
+  ignore("simple udf") {


Would you mind ignore this one in a in an independent pr first?

I created SPARK-42665 yesterday because it still test fail in 3.4.0 RC2 and be reported in the dev mail list

cc @hvanhovell should we ignore this test first? Maven test must fail due to the function is org.apache.spark.sql.ClientE2ETestSuite$$Lambda$XXX, but there is no org.apache.spark.sql.ClientE2ETestSuite in server side when test using maven.

LuciferYang · 2023-03-04T02:08:54Z

Thanks for your work @zhenlineo
If you don't mind, please give me more time to think about this pr ：）

LuciferYang · 2023-03-05T07:22:44Z

In the pr description, build/mvn compile -pl connector/connect/client/jvm should be build/mvn compile -pl connector/connect/client/jvm -am ?

LuciferYang · 2023-03-05T13:38:57Z

On the whole, it is good for me. There is only one question. Spark still uses maven for version release and deploy. But after this pr, the E2E test change to use sbt assembly server jar instead of maven shaded server jar for testing, which may weaken the maven test. We may need other ways to ensure the correctness of maven shaded server jar.

In the future, we may use sbt to completely replace maven(should not be in Spark 3.4.0), including version release, deploy and other help tools, which will no longer be a problem at that time.

LuciferYang · 2023-03-05T14:17:50Z

There is another problem that needs to be confirmed, which may not related to current pr: if other Suites inherit RemoteSparkSession, they will share the same connect server, right? (SparkConnectServerUtils is an object, so SparkConnect will only submit once)

zhenlineo · 2023-03-06T16:29:18Z

@LuciferYang Thanks for your review. This PR was trying to simplify the test running steps. But as you said it make the maven commands to call sbt implicitly. I will split the changes into smaller PRs to allow this PR only deal with the IT command change. Then we can votes if we like this change or not :)

zhenlineo · 2023-03-06T17:12:38Z

#40304 #40303

LuciferYang · 2023-03-09T03:22:25Z

seems SimpleSparkConnectService startup failed, the error message is

Error: Missing application resource.

Usage: spark-submit [options] <app jar | python file | R file> [app arguments]
Usage: spark-submit --kill [submission ID] --master [spark://...]
Usage: spark-submit --status [submission ID] --master [spark://...]
Usage: spark-submit run-example [options] example-class [example args]

Options:
  --master MASTER_URL         spark://host:port, mesos://host:port, yarn,
                              k8s://https://host:port, or local (Default: local[*]).
  --deploy-mode DEPLOY_MODE   Whether to launch the driver program locally ("client") or
                              on one of the worker machines inside the cluster ("cluster")
                              (Default: client).
  --class CLASS_NAME          Your application's main class (for Java / Scala apps).
  --name NAME                 A name of your application.
  --jars JARS                 Comma-separated list of jars to include on the driver
...

…le command

zhenlineo · 2023-03-10T04:26:31Z

seems SimpleSparkConnectService startup failed, the error message is

Error: Missing application resource.

Usage: spark-submit [options] <app jar | python file | R file> [app arguments]
Usage: spark-submit --kill [submission ID] --master [spark://...]
Usage: spark-submit --status [submission ID] --master [spark://...]
Usage: spark-submit run-example [options] example-class [example args]

Options:
  --master MASTER_URL         spark://host:port, mesos://host:port, yarn,
                              k8s://https://host:port, or local (Default: local[*]).
  --deploy-mode DEPLOY_MODE   Whether to launch the driver program locally ("client") or
                              on one of the worker machines inside the cluster ("cluster")
                              (Default: client).
  --class CLASS_NAME          Your application's main class (for Java / Scala apps).
  --name NAME                 A name of your application.
  --jars JARS                 Comma-separated list of jars to include on the driver
...

Yeah, this was caused by the bug we had in the scripts.

zhenlineo · 2023-03-10T04:28:59Z

@hvanhovell Want to keep this or shall we skip? It helps a bit when not knowing build/sbt -Pconnect -Phive package before running the IT.

github-actions bot added BUILD CONNECT SQL labels Mar 4, 2023

zhenlineo changed the title ~~[SPARK-42215][CONNECT] Single command to run Scala Client IT tests~~ [SPARK-42215][CONNECT] Simplify Scala Client IT tests Mar 4, 2023

LuciferYang reviewed Mar 4, 2023

View reviewed changes

zhenlineo force-pushed the one-cmd-for-it branch 2 times, most recently from ba43059 to d1738ac Compare March 8, 2023 21:36

Use the new spark-connect script to make the IT tests run with a sing…

874bc67

…le command

zhenlineo force-pushed the one-cmd-for-it branch from d1738ac to 874bc67 Compare March 10, 2023 04:24

zhenlineo closed this Apr 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-42215][CONNECT] Simplify Scala Client IT tests #40274

[SPARK-42215][CONNECT] Simplify Scala Client IT tests #40274

zhenlineo commented Mar 4, 2023 •

edited

zhenlineo commented Mar 4, 2023

zhenlineo commented Mar 4, 2023

LuciferYang Mar 4, 2023

LuciferYang Mar 4, 2023

LuciferYang Mar 6, 2023 •

edited

LuciferYang commented Mar 4, 2023

LuciferYang commented Mar 5, 2023

LuciferYang commented Mar 5, 2023

LuciferYang commented Mar 5, 2023

zhenlineo commented Mar 6, 2023

zhenlineo commented Mar 6, 2023

LuciferYang commented Mar 9, 2023

zhenlineo commented Mar 10, 2023

zhenlineo commented Mar 10, 2023

[SPARK-42215][CONNECT] Simplify Scala Client IT tests #40274

[SPARK-42215][CONNECT] Simplify Scala Client IT tests #40274

Conversation

zhenlineo commented Mar 4, 2023 • edited

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?

zhenlineo commented Mar 4, 2023

zhenlineo commented Mar 4, 2023

LuciferYang Mar 4, 2023

Choose a reason for hiding this comment

LuciferYang Mar 4, 2023

Choose a reason for hiding this comment

LuciferYang Mar 6, 2023 • edited

Choose a reason for hiding this comment

LuciferYang commented Mar 4, 2023

LuciferYang commented Mar 5, 2023

LuciferYang commented Mar 5, 2023

LuciferYang commented Mar 5, 2023

zhenlineo commented Mar 6, 2023

zhenlineo commented Mar 6, 2023

LuciferYang commented Mar 9, 2023

zhenlineo commented Mar 10, 2023

zhenlineo commented Mar 10, 2023

zhenlineo commented Mar 4, 2023 •

edited

LuciferYang Mar 6, 2023 •

edited