Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[TOREE-552] Downgrade Guava 14.0.1 to align with Spark #220

Merged
merged 7 commits into from
Oct 1, 2023

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Aug 29, 2023

What changes were proposed in this pull request?

This PR proposes to downgrade Guava 14.0.1 to align with Apache Spark.

The previous Guava upgrade is to make the ClassPath compatible with Java 11, google/guava#3249. As a workaround, this PR fork a ClassPath from Guava v32.1.2 to make it happy with Java 11.

Why are the changes needed?

Upgrading Guava in Spark is a "Mission Impossible" at the current stage. See details at SPARK-44811 SPARK-36676

The current tests do not cover the use cases of Spark with Hive, and based on the above Spark tickets, the Guava upgrading would break Hive usage

val sparkAll = Def.setting{
Seq(
sparkCore.value % "provided" excludeAll(
// Exclude netty (org.jboss.netty is for 3.2.2.Final only)
Copy link
Member Author

Choose a reason for hiding this comment

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

The recent version of Spark does not depend on Netty 3.x, so this exclusion is redundant.

https://mvnrepository.com/artifact/org.apache.spark/spark-core_2.12/3.3.2

@pan3793 pan3793 marked this pull request as draft August 29, 2023 14:19
@pan3793 pan3793 changed the title [TOREE-552] Downgrade Guava 14.0.1 to align with Spark [WIP][TOREE-552] Downgrade Guava 14.0.1 to align with Spark Aug 29, 2023
@pan3793
Copy link
Member Author

pan3793 commented Aug 29, 2023

CI reports failures, investigating

Comment on lines +73 to +78
// This class is derived from Guava v32.1.2
// https://github.com/google/guava/blob/v32.1.2/guava/src/com/google/common/reflect/ClassPath.java
//
// TOREE-552: Apache Toree should use Guava 14 to align with Apache Spark, unfortunately, ClassPath
// from Guava 14 does not work well with Java 11+.
// See detail at: https://github.com/google/guava/issues/3249
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a simple copy with limited changes:

  1. switch Logger from JDK logging to SFL4J
  2. directly call System.getProperty to get path.separator and java.class.path instead of using com.google.common.base.StandardSystemProperty(since Guava 15)

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an exception because it is the first Java file in the project, though sbt works well with Java source files, if requested, I can translate it to Scala.

@pan3793 pan3793 marked this pull request as ready for review August 29, 2023 19:57
@pan3793 pan3793 changed the title [WIP][TOREE-552] Downgrade Guava 14.0.1 to align with Spark [TOREE-552] Downgrade Guava 14.0.1 to align with Spark Aug 29, 2023
@pan3793
Copy link
Member Author

pan3793 commented Aug 29, 2023

@lresende would you mind taking a look?

@lresende
Copy link
Member

lresende commented Sep 4, 2023

So, #194 updated Guava, and why now, when we downgrade to the same previous version, do we need some new workarounds? If Spark uses this, can't we import the additional files from Spark?

@pan3793
Copy link
Member Author

pan3793 commented Sep 4, 2023

when we downgrade to the same previous version, do we need some new workarounds?

Yes, I explained in the PR description. "As a workaround, this PR fork a ClassPath from Guava v32.1.2 to make it happy with Java 11."

If Spark uses this, can't we import the additional files from Spark?

In short, Spark 3.x must use Guava 14.0.1. The restriction comes from the transitive deps of Spark, including Hive and Hadoop, see details in discussions in SPARK-44811 SPARK-36676. Since Guava 14's ClassPath(used by Toree) is not compatible with Java 11, we should fork ClassPath from a higher version of Guava instead.

@lresende lresende merged commit f6c2c6d into apache:master Oct 1, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants