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.

Sign up for GitHub

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

Jump to bottom

[HUDI-6508] Fix compile errors with JDK11 #9300

Closed
wants to merge 1 commit into from
Closed

[HUDI-6508] Fix compile errors with JDK11 #9300

wants to merge 1 commit into from

Conversation

Zouxxyy
Copy link
Contributor

@Zouxxyy Zouxxyy commented Jul 27, 2023 •
edited
Loading

Change Logs

Currently, JDK11 compilation will occur in the current CI (Some PRs failed for this, and it seems to appear randomly) . In order not to block merge, and support JDK11 in the future:

  • Fix compile errors with JDK11
  • build hudi with JDK 11 at test-spark-java17 in CI (when we have a java11 pipline, move to there)

Impact

Fix compile errors with JDK11

Risk level (write none, low medium or high below)

none

Documentation Update

none

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@Zouxxyy Zouxxyy changed the title [MINOR] Fix compilation errors in JDK11 [MINOR] Fix compile errors with JDK11 Jul 27, 2023
@Zouxxyy Zouxxyy closed this Jul 27, 2023
@Zouxxyy Zouxxyy reopened this Jul 27, 2023
@Zouxxyy Zouxxyy changed the title [MINOR] Fix compile errors with JDK11 [HUDI-6599] Fix compile errors with JDK11 Jul 27, 2023
@Zouxxyy Zouxxyy marked this pull request as ready for review July 27, 2023 14:09
Zouxxyy
Zouxxyy commented Jul 27, 2023
View reviewed changes
hudi-examples/hudi-examples-common/pom.xml
@@ -43,16 +43,6 @@
<plugin>
<groupId>net.alchim31.maven</groupId>
<artifactId>scala-maven-plugin</artifactId>
<executions>
Copy link
Contributor Author

@Zouxxyy Zouxxyy Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

will build failue at JDK11, see davidB/scala-maven-plugin#234

Note: this module does not need the scala plugin at all, so just remove it, like #8336

@CTTY
Copy link
Contributor

CTTY commented Jul 28, 2023

Hi @Zouxxyy , thanks for posting this PR! I think we've had an existing JIRA to track this issue: https://issues.apache.org/jira/browse/HUDI-6508

Could you use this to track the compile time support instead please?

@Zouxxyy Zouxxyy changed the title [HUDI-6599] Fix compile errors with JDK11 [HUDI-6508] Fix compile errors with JDK11 Jul 28, 2023
@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Jul 28, 2023 •
edited
Loading

Could you use this to track the compile time support instead please?

@CTTY Yes, done.

In addition, do you understand why JDK11 compilation occurs in the current CI (Some PRs failed for this, and it seems to appear randomly) before this PR ~

@CTTY
Copy link
Contributor

CTTY commented Jul 28, 2023

@Zouxxyy

Hmm I've seen this before when we were using actions/checkout@v2 in bot.yml, so I updated that to v3:

hudi/.github/workflows/bot.yml

Line 32 in cb8ff26

- uses: actions/checkout@v3

But I've not seen this error again since that. It looks like a random bug with GitHub CI

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Jul 28, 2023 •
edited
Loading

But I've not seen this error again since that. It looks like a random bug with GitHub CI

@CTTY Ohh, the error seems still occurs after update to checkout@v3. Anyway, let's fix the compilation errors first

Zouxxyy
Zouxxyy commented Jul 28, 2023
View reviewed changes
...atasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestMetadataRecordIndex.scala Outdated
@@ -158,8 +155,7 @@ class TestMetadataRecordIndex extends HoodieSparkClientTestBase {
}

private def getWriteConfig(hudiOpts: Map[String, String]): HoodieWriteConfig = {
val props = new Properties()
props.putAll(hudiOpts.asJava)
val props = TypedProperties.fromMap(hudiOpts.asJava)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #8511

Zouxxyy
Zouxxyy commented Jul 28, 2023
View reviewed changes
.github/workflows/bot.yml
@@ -126,10 +126,10 @@ jobs:

steps:
- uses: actions/checkout@v3
- name: Set up JDK 8
- name: Set up JDK 11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we have a java11 pipline, move to there

@yihua
Copy link
Contributor

yihua commented Jul 28, 2023

Let's stick to Java 8 compilation for now to avoid any issue for the upcoming 0.14.0 release.

I think @CTTY is also working on making Hudi compilation working on Java 17.

I also have a WIP branch to make Java 17 build master...yihua:hudi:java17-build Shall we merge the effort here?

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Jul 28, 2023 •
edited
Loading

Let's stick to Java 8 compilation for now to avoid any issue for the upcoming 0.14.0 release.

This PR is just build with java11 at java17 test pipeline, and without any package upgrade or code logic change, I think it's harmless (CI is pass and I also tested locally with my jdk11) when we have a java11 test pipline, move it to there.

I fixed it mainly because some PRs will be blocked currently. e.g. see
https://github.com/apache/hudi/actions/runs/5676972993/job/15384528645?pr=9293

I also have a WIP branch to make Java 17 build master...yihua:hudi:java17-build Shall we merge the effort here?

Yeah, ok, looks like the changes are basically the same, except for the upgrade of scala-maven-plugin.version and scala version

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Jul 29, 2023

@yihua @CTTY @danny0405 Hi, can you help with a review~ Many PRs have compilation errors now

@bvaradar
Copy link
Contributor

bvaradar commented Nov 8, 2023

@Zouxxyy : Can you fix the merge conflicts ?

@bvaradar bvaradar self-assigned this Nov 8, 2023
@hudi-bot
Copy link

hudi-bot commented Nov 8, 2023

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Nov 9, 2023

@Zouxxyy : Can you fix the merge conflicts ?

done

@github-actions github-actions bot added the size:S PR with lines of changes in (10, 100] label Feb 26, 2024
@yihua
Copy link
Contributor

yihua commented Jun 20, 2024

@Zouxxyy Given the conflicts with master and improvements in CI, I've created a new PR #11479 based on this one and my Java 17 branch (almost same changes) to focus on Java 11 compilation only. Closing this one.

@yihua yihua closed this Jun 20, 2024
This was referenced Jun 21, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bvaradar bvaradar

Labels
size:S PR with lines of changes in (10, 100]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Zouxxyy @CTTY @yihua @bvaradar @hudi-bot