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

Bump Iceberg from 1.6.1 to 1.7.0 #6804

Closed
wants to merge 3 commits into from
Closed

Bump Iceberg from 1.6.1 to 1.7.0 #6804

wants to merge 3 commits into from

Conversation

pionCham
Copy link
Contributor

@pionCham pionCham commented Nov 12, 2024 •
edited by bowenliang123
Loading

🔍 Description

Issue References 🔗

Apache Iceberg 1.7.0 release https://github.com/apache/iceberg/releases/tag/apache-iceberg-1.7.0

Describe Your Solution 🔧

  • Bump Apache Iceberg to 1.7.0
  • As Apache Iceberg 1.7.0 drops support for Java 8 and building with Java 11, keep it in 1.6.x for Java 8

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

Be nice. Be informative.

pan3793
pan3793 previously approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

LGTM if tests pass, thanks for your contribution!

@bowenliang123
Copy link
Contributor

Iceberg 1.7.0 has dropped the support for Java 8. Let's see whether it's passing the CI tests.

@pan3793
Copy link
Member

pan3793 commented Nov 12, 2024

Oh, it drops Java 8 ...

@pan3793 pan3793 dismissed their stale review November 12, 2024 08:47

Iceberg 1.7 drops support for Java 8

@bowenliang123
Copy link
Contributor

Oops ... it failed. How about bumping Iceberg to 1.7.0 for all except for the java-8 profile ?

@pionCham pionCham marked this pull request as draft November 13, 2024 01:35
@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (dddb037) to head (0896ac7).
Report is 4 commits behind head on master.

Additional details and impacted files 
@@          Coverage Diff           @@
##           master   #6804   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         687     687           
  Lines       42442   42442           
  Branches     5793    5793           
======================================
  Misses      42442   42442

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pionCham
Copy link
Contributor Author

Oops ... it failed. How about bumping Iceberg to 1.7.0 for all except for the java-8 profile ?
It sounded good, I tried it.

@pionCham pionCham marked this pull request as ready for review November 13, 2024 05:45
@pionCham
Copy link
Contributor Author

LGTM if tests pass, thanks for your contribution!

Tests look like they've passed .

bowenliang123
bowenliang123 approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM. As Iceberg is not shipped with Kyuubi distribution, it's alright to test with different Iceberg versions on 8 and 8+.

pan3793
pan3793 reviewed Nov 13, 2024
View reviewed changes
docker/playground/.env Outdated
@@ -18,7 +18,7 @@
AWS_JAVA_SDK_VERSION=1.12.367
HADOOP_VERSION=3.3.6
HIVE_VERSION=2.3.9
ICEBERG_VERSION=1.6.1
ICEBERG_VERSION=1.7.0
Copy link
Member

Choose a reason for hiding this comment

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

I remember the playground uses Java 8

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this. I found no clue for it. If it's using Java 8, we shall not bump the iceberg version here.

Copy link
Member

Choose a reason for hiding this comment

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

the JDK is defined at https://github.com/apache/kyuubi/blob/master/docker/playground/image/kyuubi-playground-base.Dockerfile#L13

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's revert this change and keep it 1.6.1 .

pan3793
pan3793 reviewed Nov 13, 2024
View reviewed changes
pom.xml
@@ -159,7 +159,7 @@
<httpcore.version>4.4.16</httpcore.version>
<hudi.version>0.15.0</hudi.version>
<hudi.artifact>hudi-spark${spark.binary.version}-bundle_${scala.binary.version}</hudi.artifact>
<iceberg.version>1.6.1</iceberg.version>
<iceberg.version>1.7.0</iceberg.version>
Copy link
Member

Choose a reason for hiding this comment

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

Currently, Kyuubi supports developing under Java 8/11/17, we should use 1.6.1 by default to avoid breaking devs who still using Java 8, and we can override iceberg.version in other java-* profiles

Copy link
Contributor

@bowenliang123 bowenliang123 Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It wont break the compatibility and development experience, as the java-8 profile is auto-activated when running with Java 8 binging the Iceberg 1.6.x for them.

@pan3793
Copy link
Member

pan3793 commented Nov 13, 2024

lgtm if CI pass

@bowenliang123 bowenliang123 changed the title Bump Iceberg to 1.7.0 Bump Iceberg from 1.6.1 to 1.7.0 Nov 13, 2024
@yaooqinn yaooqinn added this to the v1.11.0 milestone Nov 14, 2024
@yaooqinn yaooqinn closed this in a4eaacd Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pan3793 pan3793 pan3793 left review comments

@bowenliang123 bowenliang123 bowenliang123 approved these changes

Assignees

@pionCham pionCham

Labels
kind:build kind:documentation Documentation is a feature!
Projects
None yet
Milestone
v1.11.0
Development

Successfully merging this pull request may close these issues.
5 participants
@pionCham @bowenliang123 @pan3793 @codecov-commenter @yaooqinn