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

Flink: Bump to Flink 1.15.3 #7059

Merged
merged 2 commits into from
Mar 16, 2023
Merged

Flink: Bump to Flink 1.15.3 #7059

merged 2 commits into from
Mar 16, 2023

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Mar 9, 2023

No description provided.

@Fokko
Copy link
Contributor Author

Fokko commented Mar 9, 2023 •
edited

Auch!

/home/runner/work/iceberg/iceberg/flink/v1.15/flink/src/main/java/org/apache/iceberg/flink/IcebergTableSink.java:73: error: incompatible types: org.apache.flink.table.connector.sink.DataStreamSinkProvider is not a functional interface

        (providerContext, dataStream) ->
        ^
    no abstract method found in interface org.apache.flink.table.connector.sink.DataStreamSinkProvider

Changed in #7059

@Fokko Fokko mentioned this pull request Mar 9, 2023
Merged
stevenzwu
stevenzwu reviewed Mar 10, 2023
View reviewed changes
flink/v1.15/flink/src/main/java/org/apache/iceberg/flink/IcebergTableSink.java
@@ -69,16 +73,20 @@ public SinkRuntimeProvider getSinkRuntimeProvider(Context context) {
List<String> equalityColumns =
tableSchema.getPrimaryKey().map(UniqueConstraint::getColumns).orElseGet(ImmutableList::of);

return (DataStreamSinkProvider)
Copy link
Contributor

@stevenzwu stevenzwu Mar 10, 2023 •
edited

Choose a reason for hiding this comment

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

I checked Flink code. Flink added a default implementation to the single abstract method in DataStreamSinkProvider. Adding a default implementation usually is not considered as a breaking change, except for FunctionalInterface in this case. But this class was never annotated with @FunctionalInterface. Maybe we shouldn't have used the lambda function here at the first place.

We should fix the lambda usage regardless if we decided to update Flink patch version for compileOnly deps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Flink jira for this change: https://issues.apache.org/jira/browse/FLINK-28322

Copy link
Contributor

@stevenzwu stevenzwu Mar 10, 2023 •
edited

Choose a reason for hiding this comment

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

With that, I am leaning toward not chasing the Flink patch release upgrades due to unnecessary overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with not chasing Flink patch releases if they require code changes. I'd love to make sure they compile and work, but I wouldn't spend time tracking down errors between patch versions for third-party projects. If code changes are required, then we should stop update to a new patch version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. What you're saying is that the new version of Iceberg won't work with 1.15.0, 1.15.1, and 1.15.2 anymore, so we should not update. Makes sense to me.

Copy link
Contributor

@stevenzwu stevenzwu Mar 15, 2023 •
edited

Choose a reason for hiding this comment

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

@Fokko I think it is still good to update code here (without bumping up Flink patch version). We should avoid using lambda function since DataStreamSinkProvider is not annotated with @FunctionalInterface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevenzwu Got it, I've removed the version bump for now.

@rdblue
Copy link
Contributor

rdblue commented Mar 14, 2023

@Fokko, I think we can skip this one since it requires code changes.

@Fokko Fokko closed this Mar 15, 2023
@Fokko Fokko reopened this Mar 16, 2023
@Fokko Fokko requested a review from stevenzwu March 16, 2023 17:55
@stevenzwu stevenzwu merged commit af8efc6 into apache:master Mar 16, 2023
@stevenzwu
Copy link
Contributor

thanks @Fokko

@Fokko Fokko deleted the fd-bump-flink-1-15 branch March 16, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdblue rdblue rdblue left review comments

@stevenzwu stevenzwu Awaiting requested review from stevenzwu

Assignees
No one assigned
Labels
build flink
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Fokko @rdblue @stevenzwu