Skip to content

Conversation

@LuciferYang
Copy link
Contributor

What changes were proposed in this pull request?

Run the following commands

build/mvn clean install -DskipTests -pl connector/connect/server -am
build/mvn test -pl connector/connect/server

then we can see the following error message due to SPARK-42555 A adds the loading org.h2.Driver in beforeAll of ProtoToParsedPlanTestSuite, but

ProtoToParsedPlanTestSuite:
*** RUN ABORTED ***
  java.lang.ClassNotFoundException: org.h2.Driver
  at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:476)
  at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
  at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
  at java.base/java.lang.Class.forName0(Native Method)
  at java.base/java.lang.Class.forName(Class.java:398)
  at org.apache.spark.util.Utils$.classForName(Utils.scala:225)
  at org.apache.spark.sql.connect.ProtoToParsedPlanTestSuite.beforeAll(ProtoToParsedPlanTestSuite.scala:68)
  at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:212)
  at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
  at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
  ...

So this pr add h2 as test dependency of connect-server module to make maven test pass.

Why are the changes needed?

Add h2 as test dependency of connect-server module to make maven test pass.

Does this PR introduce any user-facing change?

No

How was this patch tested?

manual check:

build/mvn clean install -DskipTests -pl connector/connect/server -am
build/mvn test -pl connector/connect/server

with this pr, all test passed

@LuciferYang
Copy link
Contributor Author

cc @HyukjinKwon fix a maven test failed of connect server module due to dependency loss

@hvanhovell
Copy link
Contributor

@HyukjinKwon @gatorsmile @cloud-fan @dongjoon-hyun this is the 101st we have broken the maven build in the last month alone. We don't test with it, but we do feel comfortable to release with it. Are we sure the dual build setup is still a thing we want to pursue? Isn't it time to start thinking about greener pastures...

@LuciferYang
Copy link
Contributor Author

LuciferYang commented Mar 7, 2023

this is the 101st we have broken the maven build in the last month alone. We don't test with it, but we do feel comfortable to release with it. Are we sure the dual build setup is still a thing we want to pursue? Isn't it time to start thinking about greener pastures...

Personally, I think we should consider migrating the build to sbt only in Spark 3.5.0, also cc @pan3793

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM

@dongjoon-hyun
Copy link
Member

For the following @hvanhovell 's question, I'd ask @srowen 's advice.

this is the 101st we have broken the maven build in the last month alone. We don't test with it, but we do feel comfortable to release with it. Are we sure the dual build setup is still a thing we want to pursue? Isn't it time to start thinking about greener pastures...

@srowen
Copy link
Member

srowen commented Mar 7, 2023

While I'm a Maven person myself, I above all have also long preferred one build over two, and an SBT build is fine with me. I actually can't recall if there were strong reasons to keep one or the other in the past; seems like there were Reasons we needed both. We do use mvn in a few places in scripts to parse out stuff about the build, like to determine when new dependencies have been added. mvn and sbt dependency resolution aren't the same, but all the more reason to just pick one, even if we have to migrate some scripts

@LuciferYang
Copy link
Contributor Author

re-triggered GA

@beliefer
Copy link
Contributor

beliefer commented Mar 8, 2023

@LuciferYang Thank you for the job. #40291 need this one.

@HyukjinKwon
Copy link
Member

Merged to master and branch-3.4.

HyukjinKwon pushed a commit that referenced this pull request Mar 8, 2023
…dule

### What changes were proposed in this pull request?
Run the following commands

```
build/mvn clean install -DskipTests -pl connector/connect/server -am
build/mvn test -pl connector/connect/server
```

then we can see the following error message due to [SPARK-42555](https://issues.apache.org/jira/browse/SPARK-42555) A adds the loading `org.h2.Driver` in `beforeAll` of `ProtoToParsedPlanTestSuite`, but

```
ProtoToParsedPlanTestSuite:
*** RUN ABORTED ***
  java.lang.ClassNotFoundException: org.h2.Driver
  at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:476)
  at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
  at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
  at java.base/java.lang.Class.forName0(Native Method)
  at java.base/java.lang.Class.forName(Class.java:398)
  at org.apache.spark.util.Utils$.classForName(Utils.scala:225)
  at org.apache.spark.sql.connect.ProtoToParsedPlanTestSuite.beforeAll(ProtoToParsedPlanTestSuite.scala:68)
  at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:212)
  at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
  at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
  ...
```

So this pr add `h2` as test dependency of connect-server module to make maven test pass.

### Why are the changes needed?
Add `h2` as test dependency of connect-server module to make maven test pass.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
manual check:
```
build/mvn clean install -DskipTests -pl connector/connect/server -am
build/mvn test -pl connector/connect/server
```
with this pr, all test passed

Closes #40317 from LuciferYang/SPARK-42700.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
(cherry picked from commit aa8b41c)
Signed-off-by: Hyukjin Kwon <[email protected]>
@LuciferYang
Copy link
Contributor Author

LuciferYang commented Mar 8, 2023

snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
…dule

### What changes were proposed in this pull request?
Run the following commands

```
build/mvn clean install -DskipTests -pl connector/connect/server -am
build/mvn test -pl connector/connect/server
```

then we can see the following error message due to [SPARK-42555](https://issues.apache.org/jira/browse/SPARK-42555) A adds the loading `org.h2.Driver` in `beforeAll` of `ProtoToParsedPlanTestSuite`, but

```
ProtoToParsedPlanTestSuite:
*** RUN ABORTED ***
  java.lang.ClassNotFoundException: org.h2.Driver
  at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:476)
  at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
  at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
  at java.base/java.lang.Class.forName0(Native Method)
  at java.base/java.lang.Class.forName(Class.java:398)
  at org.apache.spark.util.Utils$.classForName(Utils.scala:225)
  at org.apache.spark.sql.connect.ProtoToParsedPlanTestSuite.beforeAll(ProtoToParsedPlanTestSuite.scala:68)
  at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:212)
  at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
  at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
  ...
```

So this pr add `h2` as test dependency of connect-server module to make maven test pass.

### Why are the changes needed?
Add `h2` as test dependency of connect-server module to make maven test pass.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
manual check:
```
build/mvn clean install -DskipTests -pl connector/connect/server -am
build/mvn test -pl connector/connect/server
```
with this pr, all test passed

Closes apache#40317 from LuciferYang/SPARK-42700.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
(cherry picked from commit aa8b41c)
Signed-off-by: Hyukjin Kwon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants