You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "juliuszsompolski (via GitHub)" <gi...@apache.org> on 2023/08/15 17:25:46 UTC
[GitHub] [spark] juliuszsompolski opened a new pull request, #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
juliuszsompolski opened a new pull request, #42441:
URL: https://github.com/apache/spark/pull/42441
### What changes were proposed in this pull request?
Currently, Spark Connect has the following types of integration tests:
* Client unit tests with a mock in process server (DummySparkConnectService with GRPC InProcessServerBuilder)
* Client unit tests with a mock RPC server (DummySparkConnectService with GRPC NettyServerBuilder)
* Server unit tests with in-process server and using various mocks
* E2E tests of real client with a server started in another process (using RemoteSparkSession)
What is lacking are E2E tests with an in-process Server (so that server state can be inspected asserted), and a real RPC client. This is impossible, because classes from `spark-connect-client-jvm` module include the client API which duplicates Spark SQL APIs of SparkSession, Dataset etc. When trying to pull a real client into the server module for testing, these classes clash.
Move the `org.apache.spark.sql.connect.client` code into new `spark-connect-client-jvm-internal` module, so that the internal SparkConnectClient code is separated from the client public API, and can be pulled into testing of the server.
Tried alternative approach in https://github.com/apache/spark/pull/42465. That works, but it's a hack.
Tried alterantive approach to depend on a shaded/relocated version of client in https://github.com/apache/spark/pull/42461, but it's not possible to get that working.
### Why are the changes needed?
For being able to use the internal client for testing of in-process server with an in-process client, but communicating over real RPC.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
All modules test and build.
I looked if more tests could be moved into jvm-internal, but most tests in the org.apache.spark.sql.client packages depend on the full client APIs nevertheless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42441:
URL: https://github.com/apache/spark/pull/42441#issuecomment-1679211390
Discussing with @hvanhovell and @grundprinzip , the preference is to move this code to connect-common instead, and avoid creating another module. Opened https://github.com/apache/spark/pull/42501 for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1293804388
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -152,8 +165,10 @@
<include>io.perfmark:*</include>
<include>org.codehaus.mojo:*</include>
<include>org.checkerframework:*</include>
+ <include>org.apache.spark:spark-connect-client-jvm-internal_${scala.binary.version}</include>
<include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
<include>org.apache.spark:spark-common-utils_${scala.binary.version}</include>
+ <include>org.apache.spark:spark-sql-api_${scala.binary.version}</include>
Review Comment:
as suggested by @LuciferYang , to also inline sql-api into the client uber-jar.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1293216847
##########
project/SparkBuild.scala:
##########
@@ -934,6 +938,90 @@ object SparkConnectClient {
}
}
+object SparkConnectClientInternal {
Review Comment:
```
object SparkConnectClientInternal {
import BuildCommons.protoVersion
lazy val settings = Seq(
// For some reason the resolution from the imported Maven build does not work for some
// of these dependendencies that we need to shade later on.
libraryDependencies ++= {
val guavaVersion =
SbtPomKeys.effectivePom.value.getProperties.get(
"connect.guava.version").asInstanceOf[String]
Seq(
"com.google.guava" % "guava" % guavaVersion,
"com.google.protobuf" % "protobuf-java" % protoVersion % "protobuf"
)
},
dependencyOverrides ++= {
val guavaVersion =
SbtPomKeys.effectivePom.value.getProperties.get(
"connect.guava.version").asInstanceOf[String]
Seq(
"com.google.guava" % "guava" % guavaVersion,
"com.google.protobuf" % "protobuf-java" % protoVersion
)
}
)
}
```
I think we can simplify it as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42441:
URL: https://github.com/apache/spark/pull/42441#issuecomment-1674031258
@hvanhovell @HyukjinKwon @grundprinzip @vicennial reaching out to any build experts :-)
What I want to achieve is being able to test a real Spark Connect server, with a real client that will connect to it over a localhost connection.
The problem with using mocks like the rest of the `connect` module tests do is that threads are different (no real GRPC threads are used), things like `setOnReadyHandler` and GRPC events like `OnReady` are different, so issues like what was surfaced in https://github.com/apache/spark/pull/42355 get missed. I want the real thing with an in-process server that I can poke (inspect the execution manager I'm adding in https://github.com/apache/spark/pull/42423), and a real thin client. I don't need the connect SparkSession, SparkConnectClient will do.
To do that, I split out the org.apache.spark.sql.connect.client code into `spark-connect-client-jvm-internal` module, that I can pull into the server with `<scope>test</scope>`. That package does not have any conflicts with the server code, like pulling in the client SparkSession would.
What I am doing appears to work. But it could be simpler and easier If I didn't need to create a new module, but could just pull that code and relocate it with shading...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1294327509
##########
connector/connect/client/jvm-internal/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -16,7 +16,7 @@
*/
package org.apache.spark.sql.connect.client
-import scala.jdk.CollectionConverters._
+import scala.collection.JavaConverters._
Review Comment:
hmm... `scala-collection-compat_2.12-2.8.1.jar` also provides `scala.jdk.CollectionConverters`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42441:
URL: https://github.com/apache/spark/pull/42441#issuecomment-1675035785
Thanks @LuciferYang for the suggestions. This was a quick-and-dirty hack yesterday to see if I can get it working at all. I don't know much about the build system, so all the pointers about what can be removed as not relevant to the spinned out module are very helpful.
What I would preferably want to do, is to not have to refactor this module out.
It would be great if I could take the whole `connect-client-jvm` module, and include it as a testing dependency in `connect`, relocated so that it doesn't conflict with the server classes. And then could refer to `shaded.org.apache.spark.sql.connect.client.SparkConnectClient` in my testing code.
I am investigating now if something like that is possible, either to define it in the `connect-client-jvm` module to export such relocated artifacts next to regular artifacts, or in the `connect` module, to relocate it when importing. Any pointers are also appreciated, as I don't know anything about the topic :-).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42441:
URL: https://github.com/apache/spark/pull/42441#issuecomment-1677833993
Thanks @LuciferYang for the help. I applied your suggestions, and then tried to minimize the jvm-internal pom further.
cc @grundprinzip @hvanhovell I need this for client-server testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1293803624
##########
connector/connect/client/jvm-internal/src/test/scala/org/apache/spark/sql/connect/client/util/DummySparkConnectService.scala:
##########
@@ -0,0 +1,194 @@
+/*
Review Comment:
This got moved from SparkConnectClientSuite.
`withNettyDummySparkConnectService` helper was added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42441:
URL: https://github.com/apache/spark/pull/42441#issuecomment-1675498338
This at least appears to work, but would need a lot of cleanup:
* make the pom.xml of the spark-connect-client-jvm-internal and spark-connect-client-jvm, and the configuration objects for SparkConnectClient and SparkConnectClientInternal SparkBuild.scala actually reflect what each of these modules needs and remove the duplication of things copied over
* move tests for org.apache.spark.connect.client to the jvm-internal module as well...
@vicennial you set this up originally in https://github.com/apache/spark/pull/39078. Do you remember the context of which dependencies and which configs are needed for which parts, and could help separate the two?
I would much rather have a simpler solution when I can just relocate it like in https://github.com/apache/spark/pull/42461, but I couldn't get that to work...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1292325302
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-parent_2.12</artifactId>
+ <version>4.0.0-SNAPSHOT</version>
+ <relativePath>../../../../pom.xml</relativePath>
+ </parent>
+
+ <artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
+ <packaging>jar</packaging>
+ <name>Spark Project Connect Client Internal</name>
+ <url>https://spark.apache.org/</url>
+ <properties>
+ <sbt.project.name>connect-client-jvm-internal</sbt.project.name>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sql-api_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sketch_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>${connect.guava.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>failureaccess</artifactId>
+ <version>${guava.failureaccess.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-codec-http2</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-handler-proxy</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-unix-common</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.lihaoyi</groupId>
+ <artifactId>ammonite_${scala.version}</artifactId>
+ <version>${ammonite.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.scalacheck</groupId>
+ <artifactId>scalacheck_${scala.binary.version}</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <!-- Use mima to perform the compatibility check -->
+ <dependency>
+ <groupId>com.typesafe</groupId>
+ <artifactId>mima-core_${scala.binary.version}</artifactId>
+ <version>${mima.version}</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <build>
+ <outputDirectory>target/scala-${scala.binary.version}/classes</outputDirectory>
+ <testOutputDirectory>target/scala-${scala.binary.version}/test-classes</testOutputDirectory>
+ <plugins>
+ <!-- Shade all Guava / Protobuf / Netty dependencies of this build -->
+ <!-- TODO (SPARK-42449): Ensure shading rules are handled correctly in `native-image.properties` and support GraalVM -->
+ <plugin>
Review Comment:
We can remove all the `maven-shade-plugin` configurations in `connect-client-jvm-internal`. This can resolve the Maven compilation failure issue like
https://github.com/juliuszsompolski/apache-spark/actions/runs/5837858385/job/15834073747
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1294040060
##########
connector/connect/client/jvm-internal/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -16,7 +16,7 @@
*/
package org.apache.spark.sql.connect.client
-import scala.jdk.CollectionConverters._
+import scala.collection.JavaConverters._
Review Comment:
lint build complained for some reason.
It seems that scala.jdk.CollectionConverters is a scala 2.13 API, but somehow it's used in a few other places in other modules and there it doesn't complain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1294040060
##########
connector/connect/client/jvm-internal/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -16,7 +16,7 @@
*/
package org.apache.spark.sql.connect.client
-import scala.jdk.CollectionConverters._
+import scala.collection.JavaConverters._
Review Comment:
lint build complained for some reason
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski closed pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski closed pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
URL: https://github.com/apache/spark/pull/42441
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1293804710
##########
connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/SparkSessionDummyIntegrationSuite.scala:
##########
@@ -0,0 +1,71 @@
+/*
Review Comment:
Two tests needing a SparkSession factored out from SparkConnectClientSuite
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1290782130
##########
connector/connect/server/src/test/scala/org/apache/spark/sql/connect/SparkConnectReattachableExecuteSuite.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect
+
+import java.util.UUID
+
+import org.apache.spark.connect.proto
+import org.apache.spark.sql.connect.client.SparkConnectClient
+import org.apache.spark.sql.connect.dsl.MockRemoteSession
+import org.apache.spark.sql.connect.dsl.plans._
+import org.apache.spark.sql.connect.service.SparkConnectService
+import org.apache.spark.sql.test.SharedSparkSession
+
+class SparkConnectReattachableExecuteSuite extends SharedSparkSession {
+
+ private val sessionId = UUID.randomUUID.toString()
+ private val userContext = proto.UserContext
+ .newBuilder()
+ .setUserId("c1")
+ .build()
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ SparkConnectService.start(spark.sparkContext)
+ }
+
+ override def afterAll(): Unit = {
+ SparkConnectService.stop()
+ }
+
+ test("test") {
+ val client = SparkConnectClient.builder().build()
+ val connect = new MockRemoteSession()
+ val plan = proto.Plan
+ .newBuilder()
+ .setRoot(connect.sql("select * from range(1000)"))
+ .build()
+ val request = proto.ExecutePlanRequest
+ .newBuilder()
+ .setUserContext(userContext)
+ .setSessionId(sessionId)
+ .setOperationId(UUID.randomUUID().toString)
+ .setPlan(plan)
+ .build()
+
+ val iter = client.execute(plan)
+ iter.next()
+ }
Review Comment:
pushed just basic validation here, because rest needs inspecting execution manager from https://github.com/apache/spark/pull/42423
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1290758670
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala:
##########
@@ -263,6 +263,7 @@ object SparkConnectService extends Logging {
private type SessionCacheKey = (String, String)
private[connect] var server: Server = _
+ private[connect] var service: SparkConnectService = _
Review Comment:
actually probably not needed. This was from before I tried to just send request to the service directly, but was not satisfied, because I was not getting The Real Experience.
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review Comment:
this is mostly a copy from jvm/pom.xml, so can surely be pruned as jvm-internal should have less deps.
##########
project/SparkBuild.scala:
##########
@@ -460,6 +463,7 @@ object SparkBuild extends PomBuild {
enable(SparkConnectCommon.settings)(connectCommon)
enable(SparkConnect.settings)(connect)
enable(SparkConnectClient.settings)(connectClient)
+ enable(SparkConnectClient.settings)(connectClientInternal)
Review Comment:
just reused the SparkConnectClient settings. Likely needs tweaking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1293233203
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,192 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
Review Comment:
```
<?xml version="1.0" encoding="UTF-8"?>
<!--
~ Licensed to the Apache Software Foundation (ASF) under one or more
~ contributor license agreements. See the NOTICE file distributed with
~ this work for additional information regarding copyright ownership.
~ The ASF licenses this file to You under the Apache License, Version 2.0
~ (the "License"); you may not use this file except in compliance with
~ the License. You may obtain a copy of the License at
~
~ http://www.apache.org/licenses/LICENSE-2.0
~
~ Unless required by applicable law or agreed to in writing, software
~ distributed under the License is distributed on an "AS IS" BASIS,
~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
~ See the License for the specific language governing permissions and
~ limitations under the License.
-->
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.spark</groupId>
<artifactId>spark-parent_2.12</artifactId>
<version>4.0.0-SNAPSHOT</version>
<relativePath>../../../../pom.xml</relativePath>
</parent>
<artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
<packaging>jar</packaging>
<name>Spark Project Connect Client Internal</name>
<url>https://spark.apache.org/</url>
<properties>
<sbt.project.name>connect-client-jvm-internal</sbt.project.name>
</properties>
<dependencies>
<dependency>
<groupId>org.apache.spark</groupId>
<artifactId>spark-connect-common_${scala.binary.version}</artifactId>
<version>${project.version}</version>
<exclusions>
<exclusion>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.spark</groupId>
<artifactId>spark-sql-api_${scala.binary.version}</artifactId>
<version>${project.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.spark</groupId>
<artifactId>spark-sketch_${scala.binary.version}</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>com.google.protobuf</groupId>
<artifactId>protobuf-java</artifactId>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>${connect.guava.version}</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>failureaccess</artifactId>
<version>${guava.failureaccess.version}</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-codec-http2</artifactId>
<version>${netty.version}</version>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
<version>${netty.version}</version>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-transport-native-unix-common</artifactId>
<version>${netty.version}</version>
</dependency>
<dependency>
<groupId>com.lihaoyi</groupId>
<artifactId>ammonite_${scala.version}</artifactId>
<version>${ammonite.version}</version>
<scope>provided</scope>
</dependency>
</dependencies>
<build>
<outputDirectory>target/scala-${scala.binary.version}/classes</outputDirectory>
<testOutputDirectory>target/scala-${scala.binary.version}/test-classes</testOutputDirectory>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
<executions>
<execution>
<id>add-sources</id>
<phase>generate-sources</phase>
<goals>
<goal>add-source</goal>
</goals>
<configuration>
<sources>
<source>src/main/scala-${scala.binary.version}</source>
</sources>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
```
I think we can simplify it as above, then maven build and testing should be able to pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1294351221
##########
connector/connect/client/jvm-internal/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -16,7 +16,7 @@
*/
package org.apache.spark.sql.connect.client
-import scala.jdk.CollectionConverters._
+import scala.collection.JavaConverters._
Review Comment:
I guess the jvm-internal module must be missing dependency on `scala-collection-compat_2.12-2.8.1.jar` somehow.
I don't see it explicit in any POM, so I guess it gets pulled trasitively by something in some of the modules with more deps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski closed pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski closed pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
URL: https://github.com/apache/spark/pull/42441
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #42441:
URL: https://github.com/apache/spark/pull/42441#issuecomment-1674069872
cc @LuciferYang too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1290854590
##########
connector/connect/client/jvm/pom.xml:
##########
@@ -46,6 +46,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-client-jvm-internal_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
Review Comment:
https://github.com/apache/spark/blob/b828ff0bdba8e4fa8506c9d1fdc4a48ec491d52a/connector/connect/client/jvm/pom.xml#L155-L156
I recommend including `connect-client-jvm-internal` in the `include` list of the `maven-shade-plugin`(like spark-connect-common and spark-common-utils). This way, at the user level, it would still be sufficient to just depend on `connect-client-jvm` uber jar
There might be some other modules that need to be included as well, such as `sql-api`, and so on. WDYT @hvanhovell
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-parent_2.12</artifactId>
+ <version>4.0.0-SNAPSHOT</version>
+ <relativePath>../../../../pom.xml</relativePath>
+ </parent>
+
+ <artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
+ <packaging>jar</packaging>
+ <name>Spark Project Connect Client Internal</name>
+ <url>https://spark.apache.org/</url>
+ <properties>
+ <sbt.project.name>connect-client-jvm-internal</sbt.project.name>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sql-api_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sketch_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>${connect.guava.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>failureaccess</artifactId>
+ <version>${guava.failureaccess.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-codec-http2</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-handler-proxy</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-unix-common</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.lihaoyi</groupId>
+ <artifactId>ammonite_${scala.version}</artifactId>
+ <version>${ammonite.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.scalacheck</groupId>
+ <artifactId>scalacheck_${scala.binary.version}</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <!-- Use mima to perform the compatibility check -->
+ <dependency>
+ <groupId>com.typesafe</groupId>
Review Comment:
The tool for mima check is still in `connect-client-jvm`, this dependency can be removed.
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-parent_2.12</artifactId>
+ <version>4.0.0-SNAPSHOT</version>
+ <relativePath>../../../../pom.xml</relativePath>
+ </parent>
+
+ <artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
+ <packaging>jar</packaging>
+ <name>Spark Project Connect Client Internal</name>
+ <url>https://spark.apache.org/</url>
+ <properties>
+ <sbt.project.name>connect-client-jvm-internal</sbt.project.name>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sql-api_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sketch_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>${connect.guava.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>failureaccess</artifactId>
+ <version>${guava.failureaccess.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-codec-http2</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-handler-proxy</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-unix-common</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.lihaoyi</groupId>
+ <artifactId>ammonite_${scala.version}</artifactId>
+ <version>${ammonite.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.scalacheck</groupId>
+ <artifactId>scalacheck_${scala.binary.version}</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <!-- Use mima to perform the compatibility check -->
+ <dependency>
+ <groupId>com.typesafe</groupId>
+ <artifactId>mima-core_${scala.binary.version}</artifactId>
+ <version>${mima.version}</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <build>
+ <outputDirectory>target/scala-${scala.binary.version}/classes</outputDirectory>
+ <testOutputDirectory>target/scala-${scala.binary.version}/test-classes</testOutputDirectory>
+ <plugins>
+ <!-- Shade all Guava / Protobuf / Netty dependencies of this build -->
+ <!-- TODO (SPARK-42449): Ensure shading rules are handled correctly in `native-image.properties` and support GraalVM -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-shade-plugin</artifactId>
+ <configuration>
+ <shadedArtifactAttached>false</shadedArtifactAttached>
+ <artifactSet>
+ <includes>
+ <include>com.google.android:*</include>
+ <include>com.google.api.grpc:*</include>
+ <include>com.google.code.findbugs:*</include>
+ <include>com.google.code.gson:*</include>
+ <include>com.google.errorprone:*</include>
+ <include>com.google.guava:*</include>
+ <include>com.google.j2objc:*</include>
+ <include>com.google.protobuf:*</include>
+ <include>io.grpc:*</include>
+ <include>io.netty:*</include>
+ <include>io.perfmark:*</include>
+ <include>org.codehaus.mojo:*</include>
+ <include>org.checkerframework:*</include>
+ <include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
+ <include>org.apache.spark:spark-common-utils_${scala.binary.version}</include>
+ </includes>
+ </artifactSet>
+ <relocations>
+ <relocation>
+ <pattern>io.grpc</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.grpc</shadedPattern>
+ <includes>
+ <include>io.grpc.**</include>
+ </includes>
+ </relocation>
+ <relocation>
+ <pattern>com.google</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.com.google</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>io.netty</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.netty</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>org.checkerframework</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.org.checkerframework</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>javax.annotation</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.javax.annotation</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>io.perfmark</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.perfmark</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>org.codehaus</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.org.codehaus</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>android.annotation</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.android.annotation</shadedPattern>
+ </relocation>
+ </relocations>
+ <!--SPARK-42228: Add `ServicesResourceTransformer` to relocation class names in META-INF/services for grpc-->
+ <transformers>
+ <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/>
+ </transformers>
+ </configuration>
+ </plugin>
+ <plugin>
Review Comment:
The configuration of the ‘maven-jar-plugin’ is a bit superfluous too at the moment
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-parent_2.12</artifactId>
+ <version>4.0.0-SNAPSHOT</version>
+ <relativePath>../../../../pom.xml</relativePath>
+ </parent>
+
+ <artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
+ <packaging>jar</packaging>
+ <name>Spark Project Connect Client Internal</name>
+ <url>https://spark.apache.org/</url>
+ <properties>
+ <sbt.project.name>connect-client-jvm-internal</sbt.project.name>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sql-api_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sketch_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>${connect.guava.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>failureaccess</artifactId>
+ <version>${guava.failureaccess.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-codec-http2</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-handler-proxy</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-unix-common</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.lihaoyi</groupId>
+ <artifactId>ammonite_${scala.version}</artifactId>
+ <version>${ammonite.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.scalacheck</groupId>
+ <artifactId>scalacheck_${scala.binary.version}</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <!-- Use mima to perform the compatibility check -->
+ <dependency>
+ <groupId>com.typesafe</groupId>
+ <artifactId>mima-core_${scala.binary.version}</artifactId>
+ <version>${mima.version}</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <build>
+ <outputDirectory>target/scala-${scala.binary.version}/classes</outputDirectory>
+ <testOutputDirectory>target/scala-${scala.binary.version}/test-classes</testOutputDirectory>
+ <plugins>
+ <!-- Shade all Guava / Protobuf / Netty dependencies of this build -->
+ <!-- TODO (SPARK-42449): Ensure shading rules are handled correctly in `native-image.properties` and support GraalVM -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-shade-plugin</artifactId>
+ <configuration>
+ <shadedArtifactAttached>false</shadedArtifactAttached>
+ <artifactSet>
+ <includes>
+ <include>com.google.android:*</include>
+ <include>com.google.api.grpc:*</include>
+ <include>com.google.code.findbugs:*</include>
+ <include>com.google.code.gson:*</include>
+ <include>com.google.errorprone:*</include>
+ <include>com.google.guava:*</include>
+ <include>com.google.j2objc:*</include>
+ <include>com.google.protobuf:*</include>
+ <include>io.grpc:*</include>
+ <include>io.netty:*</include>
+ <include>io.perfmark:*</include>
+ <include>org.codehaus.mojo:*</include>
+ <include>org.checkerframework:*</include>
+ <include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
+ <include>org.apache.spark:spark-common-utils_${scala.binary.version}</include>
+ </includes>
+ </artifactSet>
+ <relocations>
+ <relocation>
+ <pattern>io.grpc</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.grpc</shadedPattern>
+ <includes>
+ <include>io.grpc.**</include>
+ </includes>
+ </relocation>
+ <relocation>
+ <pattern>com.google</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.com.google</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>io.netty</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.netty</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>org.checkerframework</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.org.checkerframework</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>javax.annotation</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.javax.annotation</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>io.perfmark</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.perfmark</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>org.codehaus</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.org.codehaus</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>android.annotation</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.android.annotation</shadedPattern>
+ </relocation>
+ </relocations>
+ <!--SPARK-42228: Add `ServicesResourceTransformer` to relocation class names in META-INF/services for grpc-->
+ <transformers>
+ <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/>
+ </transformers>
+ </configuration>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>prepare-test-jar</id>
+ <phase>test-compile</phase>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>build-helper-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>add-sources</id>
+ <phase>generate-sources</phase>
+ <goals>
+ <goal>add-source</goal>
+ </goals>
+ <configuration>
+ <sources>
+ <source>src/main/scala-${scala.binary.version}</source>
+ </sources>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+</project>
Review Comment:
Needs an empty line :)
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-parent_2.12</artifactId>
+ <version>4.0.0-SNAPSHOT</version>
+ <relativePath>../../../../pom.xml</relativePath>
+ </parent>
+
+ <artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
+ <packaging>jar</packaging>
+ <name>Spark Project Connect Client Internal</name>
+ <url>https://spark.apache.org/</url>
+ <properties>
+ <sbt.project.name>connect-client-jvm-internal</sbt.project.name>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sql-api_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sketch_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>${connect.guava.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>failureaccess</artifactId>
+ <version>${guava.failureaccess.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-codec-http2</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-handler-proxy</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-unix-common</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.lihaoyi</groupId>
+ <artifactId>ammonite_${scala.version}</artifactId>
+ <version>${ammonite.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.scalacheck</groupId>
+ <artifactId>scalacheck_${scala.binary.version}</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <!-- Use mima to perform the compatibility check -->
+ <dependency>
+ <groupId>com.typesafe</groupId>
+ <artifactId>mima-core_${scala.binary.version}</artifactId>
+ <version>${mima.version}</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <build>
+ <outputDirectory>target/scala-${scala.binary.version}/classes</outputDirectory>
+ <testOutputDirectory>target/scala-${scala.binary.version}/test-classes</testOutputDirectory>
+ <plugins>
+ <!-- Shade all Guava / Protobuf / Netty dependencies of this build -->
+ <!-- TODO (SPARK-42449): Ensure shading rules are handled correctly in `native-image.properties` and support GraalVM -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-shade-plugin</artifactId>
+ <configuration>
+ <shadedArtifactAttached>false</shadedArtifactAttached>
+ <artifactSet>
+ <includes>
+ <include>com.google.android:*</include>
+ <include>com.google.api.grpc:*</include>
+ <include>com.google.code.findbugs:*</include>
+ <include>com.google.code.gson:*</include>
+ <include>com.google.errorprone:*</include>
+ <include>com.google.guava:*</include>
+ <include>com.google.j2objc:*</include>
+ <include>com.google.protobuf:*</include>
+ <include>io.grpc:*</include>
+ <include>io.netty:*</include>
+ <include>io.perfmark:*</include>
+ <include>org.codehaus.mojo:*</include>
+ <include>org.checkerframework:*</include>
+ <include>org.apache.spark:spark-connect-common_${scala.binary.version}</include>
+ <include>org.apache.spark:spark-common-utils_${scala.binary.version}</include>
+ </includes>
+ </artifactSet>
+ <relocations>
+ <relocation>
+ <pattern>io.grpc</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.grpc</shadedPattern>
+ <includes>
+ <include>io.grpc.**</include>
+ </includes>
+ </relocation>
+ <relocation>
+ <pattern>com.google</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.com.google</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>io.netty</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.netty</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>org.checkerframework</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.org.checkerframework</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>javax.annotation</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.javax.annotation</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>io.perfmark</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.io.perfmark</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>org.codehaus</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.org.codehaus</shadedPattern>
+ </relocation>
+ <relocation>
+ <pattern>android.annotation</pattern>
+ <shadedPattern>${spark.shade.packageName}.connect.client.android.annotation</shadedPattern>
+ </relocation>
+ </relocations>
+ <!--SPARK-42228: Add `ServicesResourceTransformer` to relocation class names in META-INF/services for grpc-->
+ <transformers>
+ <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/>
+ </transformers>
+ </configuration>
+ </plugin>
+ <plugin>
Review Comment:
The configuration of the ‘maven-jar-plugin’ is a bit superfluous too at the moment
##########
project/SparkBuild.scala:
##########
@@ -460,6 +463,7 @@ object SparkBuild extends PomBuild {
enable(SparkConnectCommon.settings)(connectCommon)
enable(SparkConnect.settings)(connect)
enable(SparkConnectClient.settings)(connectClient)
+ enable(SparkConnectClient.settings)(connectClientInternal)
Review Comment:
I would still suggest defining a separate setting object for `connectClientInternal`, as many configurations in `SparkConnectClient` may be useless for the `connectClientInternal` module, such as `buildTestDeps`, `grpc plugin`, and so on.
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-parent_2.12</artifactId>
+ <version>4.0.0-SNAPSHOT</version>
+ <relativePath>../../../../pom.xml</relativePath>
+ </parent>
+
+ <artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
+ <packaging>jar</packaging>
+ <name>Spark Project Connect Client Internal</name>
+ <url>https://spark.apache.org/</url>
+ <properties>
+ <sbt.project.name>connect-client-jvm-internal</sbt.project.name>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sql-api_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sketch_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>${connect.guava.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>failureaccess</artifactId>
+ <version>${guava.failureaccess.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-codec-http2</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-handler-proxy</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-unix-common</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.lihaoyi</groupId>
+ <artifactId>ammonite_${scala.version}</artifactId>
+ <version>${ammonite.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
Review Comment:
Since this module doesn't have test codes yet, all these test-scope dependencies can be deleted for the time being. They can be added again when there's a real need.
##########
connector/connect/client/jvm-internal/pom.xml:
##########
@@ -0,0 +1,235 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-parent_2.12</artifactId>
+ <version>4.0.0-SNAPSHOT</version>
+ <relativePath>../../../../pom.xml</relativePath>
+ </parent>
+
+ <artifactId>spark-connect-client-jvm-internal_2.12</artifactId>
+ <packaging>jar</packaging>
+ <name>Spark Project Connect Client Internal</name>
+ <url>https://spark.apache.org/</url>
+ <properties>
+ <sbt.project.name>connect-client-jvm-internal</sbt.project.name>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sql-api_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-sketch_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>${connect.guava.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>failureaccess</artifactId>
+ <version>${guava.failureaccess.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-codec-http2</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-handler-proxy</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-unix-common</artifactId>
+ <version>${netty.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.lihaoyi</groupId>
+ <artifactId>ammonite_${scala.version}</artifactId>
+ <version>${ammonite.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-connect-common_${scala.binary.version}</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ <exclusions>
+ <exclusion>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.scalacheck</groupId>
+ <artifactId>scalacheck_${scala.binary.version}</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <!-- Use mima to perform the compatibility check -->
+ <dependency>
+ <groupId>com.typesafe</groupId>
+ <artifactId>mima-core_${scala.binary.version}</artifactId>
+ <version>${mima.version}</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <build>
+ <outputDirectory>target/scala-${scala.binary.version}/classes</outputDirectory>
+ <testOutputDirectory>target/scala-${scala.binary.version}/test-classes</testOutputDirectory>
+ <plugins>
+ <!-- Shade all Guava / Protobuf / Netty dependencies of this build -->
+ <!-- TODO (SPARK-42449): Ensure shading rules are handled correctly in `native-image.properties` and support GraalVM -->
+ <plugin>
Review Comment:
Are all the shaded rules necessary? Or is it possible not to configure shaded?
I have performed a local Maven build and used Maven to test the added cases. When retaining the `maven-shade-plugin`, both the build of the client module and the newly added test cases in the server module would fail. However, upon the removal of the related configuration of `maven-shade-plugin`, everything succeeded.
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala:
##########
@@ -263,6 +263,7 @@ object SparkConnectService extends Logging {
private type SessionCacheKey = (String, String)
private[connect] var server: Server = _
+ private[connect] var service: SparkConnectService = _
Review Comment:
hmm, If it's just for testing, it doesn't seem necessary. After removing these changes, the tests will also be successful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on pull request #42441: [CONNECT][POC] Have real server and real simple client in tests
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #42441:
URL: https://github.com/apache/spark/pull/42441#issuecomment-1675413010
I try to make a shaded copy of client module in https://github.com/apache/spark/pull/42461, but can't get that to work...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1293802942
##########
connector/connect/client/jvm-internal/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala:
##########
@@ -0,0 +1,218 @@
+/*
Review Comment:
This file has been mostly moved, but
* DummySparkConnectService was moved to it's own util file
* two tests were moved to SparkSessionDummyIntegrationSuite
and it makes github see it as a full diff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #42441: [SPARK-44806][CONNECT] Separate `connect-client-jvm-internal` module to be able to test real in-process server with a real RPC client
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #42441:
URL: https://github.com/apache/spark/pull/42441#discussion_r1294327509
##########
connector/connect/client/jvm-internal/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -16,7 +16,7 @@
*/
package org.apache.spark.sql.connect.client
-import scala.jdk.CollectionConverters._
+import scala.collection.JavaConverters._
Review Comment:
hmm... `scala-collection-compat_2.12-2.8.1.jar` also provides `scala.jdk.CollectionConverters` , but `scala.collection.JavaConverters` is also fine
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org