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

Support building with Java 21 #10474

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Support building with Java 21 #10474

wants to merge 2 commits into from

Conversation

findepi
Copy link
Collaborator

@findepi findepi commented Jun 10, 2024

This is currently the latest Java LTS version, so we should support
building with it.

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
.github/workflows/java-ci.yml Outdated Show resolved Hide resolved
.java-version Outdated Show resolved Hide resolved
@findepi findepi force-pushed the findepi/21 branch 3 times, most recently from 5b03499 to f564c36 Compare June 11, 2024 08:50
@findepi findepi force-pushed the findepi/21 branch 5 times, most recently from 7ca0b11 to d9bae17 Compare June 11, 2024 13:03
@github-actions github-actions bot added the hive label Jun 11, 2024
@findepi findepi marked this pull request as ready for review June 11, 2024 19:56
@jbonofre
Copy link
Member

@findepi thanks. FYI, I'm fixing the gradle-revapi plugin and Ed and I will do the release, then, I will rebase the Gradle 8.8 PR and we are good 😄

build.gradle Show resolved Hide resolved
@@ -59,6 +59,10 @@ project(":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}") {
implementation project(':iceberg-parquet')
implementation project(':iceberg-arrow')
implementation("org.scala-lang.modules:scala-collection-compat_${scalaVersion}:${libs.versions.scala.collection.compat.get()}")
if (scalaVersion == '2.12') {
// scala-collection-compat_2.12 pulls scala 2.12.17 and we need 2.12.18 for JDK 21 support
Copy link
Contributor

Choose a reason for hiding this comment

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

does this also apply for Scala 2.13?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

apparently not, i think the Ci would detect that

@findepi findepi marked this pull request as ready for review June 28, 2024 10:20
@findepi findepi requested review from nastra and Fokko June 28, 2024 10:20
@findepi
Copy link
Collaborator Author

findepi commented Jun 28, 2024

Finally not draft anymore!

apply plugin: 'com.diffplug.spotless'
// We need to update Google Java Format to 1.17.0+ to run spotless on JDK 8, but that requires dropping support for JDK 8.
if (JavaVersion.current() != JavaVersion.VERSION_21) {
apply plugin: 'com.diffplug.spotless'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can merge the PR without having spotless enabled unfortunately

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wish this was avoidable. I do not know of a way to run same formatting rules under Java 8 and Java 21 and that's we support today.
I do believe that being able to test and actually testing with 21 is incremental improvement over current state. I see value in adding even partial support for 21. In particular, this PR identifies a problem with scala 2.12.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you bring this topic up on the Dev mailing list to see what others think about adding JDK21 support without being able to run spotless?

Copy link
Member

Choose a reason for hiding this comment

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

I think the comment is not accurate: we need to update Google Java Format to 1.17.0+ to run spotless on JDK21, but it breaks JDK8.
I'm more in favor of jumping to JDK21 with spotless/GJF 1.17.0+, as soon as we don't have any module requiring JDK8.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jbonofre we can't upgrade the GJF as long as we support JDK8 because upgrading the GJF version will produce different results depending on the underlying JDK it is executed with and so CI actions (and locally) will fail.

Copy link
Member

Choose a reason for hiding this comment

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

That's my point: I would postpone the JDK21 update right now, and jump directly to JDK21 without supporting JDK8 anymore.
I think it will be very difficult to support both JDK21 and JDK8 from spotless/GJF standpoint. So, I would directly jump to new version when possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping JDK8 support means dropping Hive support (which I'm totally fine with) but we need to properly communicate which Iceberg version will drop Hive support. My understanding was that we were planning to drop Hive support with Iceberg 2.0

Copy link
Member

Choose a reason for hiding this comment

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

Yes exactly. It's what I proposed while ago in the Iceberg 2.0 discussion thread. So, I propose:

  • on 1.x (including 1.6.0 release currently in preparation), we keep JDK8
  • for 2.x, we merge this PR and jump to JDK21

I can reply on the Iceberg 2.0 thread if you want or Piotr can start a new thread about JDK21 on Iceberg 2.0.
I would love to have feedback on the Iceberg 2.0 thread 😄

Copy link

@BsoBird BsoBird Jul 2, 2024

Choose a reason for hiding this comment

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

Dropping JDK8 support means dropping Hive support (which I'm totally fine with) but we need to properly communicate which Iceberg version will drop Hive support. My understanding was that we were planning to drop Hive support with Iceberg 2.0

Sir. Maybe the iceberg-hive module should be maintained by the hive community in the hive project. Because hive 3.X will EOL sooner or later.

Or just keep the catalog support.

Copy link
Contributor

Choose a reason for hiding this comment

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

@BsoBird newer versions of Hive are maintained by the Hive community but the Iceberg community still maintains an older version of Hive

spark: ['3.3', '3.4', '3.5']
scala: ['2.12', '2.13']
exclude:
# Spark 3.5 is the first version supporting Java 21 (https://issues.apache.org/jira/browse/SPARK-42369)
Copy link
Member

Choose a reason for hiding this comment

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

Actually Spark 3.5 does not fully support Java 21, the right ticket is https://issues.apache.org/jira/browse/SPARK-43831

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @pan3793 for your comment and the link. This is good reference.
If our tests with Spark 3.5 pass with Java 21, we don't need to disable them, but it's good to know we should be expecting challenges. I will rework the comment.

This is currently the latest Java LTS version, so we should support
building with it.
@findepi
Copy link
Collaborator Author

findepi commented Jul 10, 2024

Since spotless plugin is not installed on JDK 21, ./gradlew spotlessApply would fail with

$ JAVA_HOME=~/.jenv/versions/21 ./gradlew spotlessApply

FAILURE: Build failed with an exception.

* What went wrong:
Task 'spotlessApply' not found in root project 'iceberg' and its subprojects.

* Try:
> Run gradlew tasks to get a list of available tasks.
> For more on name expansion, please refer to https://docs.gradle.org/8.8/userguide/command_line_interface.html#sec:name_abbreviation in the Gradle documentation.
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
> Get more help at https://help.gradle.org.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.8/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD FAILED in 14s

Added spotlessApply stub, for more actionable message:

$ JAVA_HOME=~/.jenv/versions/21 ./gradlew spotlessApply

FAILURE: Build failed with an exception.

* Where:
Script '/Users/findepi/repos/iceberg/baseline.gradle' line: 51

* What went wrong:
A problem occurred evaluating script.
> Spotless plugin is currently disabled when running on JDK 21 (until we drop JDK 8). To run spotlessApply please use a different JDK version.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
> Get more help at https://help.gradle.org.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.8/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD FAILED in 3s

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.

None yet

6 participants