Flink: Upgrade to flink 1.13.2 #3116

Flyangz · 2021-09-14T17:09:46Z

This job is for #2558 and is completed on the basis of #2629 , so thanks for the contribution of @zhangjun0x01 .
Below are some explanations:

The options are not checked in FlinkCatalogFactory.createCatalog(), this is to be compatible with the old implementation. And this way we can use customized options as before without pre-settings.
The implementation is not compatible with Flink 1.12 because CatalogFactory interface does not extend Factory interface in 1.12.

kbendick · 2021-09-14T18:48:13Z

The options are not checked in FlinkCatalogFactory.createCatalog(), this is to be compatible with the old implementation. And this way we can use customized options as before without pre-settings.

+1 to this. We have a number of customized options for things like AWS s3 / IAM etc that go on catalogs and we need to be able to support them.

The implementation is not compatible with Flink 1.12 because CatalogFactory interface does not extend Factory interface in 1.12.

Does this mean we're dropping support entirely for Flink 1.12 in the next release (and effectively in the master branch)? This seems potentially premature. I thought that the Flink 1.13 interface added Factory, but kept the old interface TableFactory as well that was extended in 1.12 for backwards compatibility.

Is it possible we can do the same (continue to implement both interfaces or have two separate classes) to keep support for Flink 1.12 @Flyangz?

wg1026688210 · 2021-09-15T05:59:49Z

we do not have the plan of improving flink to 1.13 recently, is there a way to support both 1.12 and 1.13 versions.

kbendick · 2021-09-15T06:04:33Z

we do not have the plan of improving flink to 1.13 recently, is there a way to support both 1.12 and 1.13 versions.

I believe that @zhangjun0x01's original PR supported both flink 1.12 and flink 1.13. Like I mentioned, if the FlinkCatalogFactory implements both APIs, will that not suffice to support both 1.12 and 1.13?

Dropping support for Flink 1.12 is a big decision in my opinion, and should be brought up on the mailing list or at the community sync-up.

Does my idea of continuing to implement TableFactory, in addition to adding the newer Factory interface functions, just like Flink's CatalogFactory does itself not work? I thought that was why Flink's CatalogFactory implemented both the old interface and the new interface, to mark the old interface as deprecated but still support both versions.

Flyangz · 2021-09-17T16:05:33Z

This committed code implements both TableFactory and Factory. Actually, Flink 1.13 will continue to use TableFactory API. So only implementing TableFactory is the method with the least changes and the lowest risk to upgrade to 1.13. Which way do you prefer? Implementing both or just TableFactory?

flink/src/main/java/org/apache/iceberg/flink/FlinkCatalogFactory.java

kbendick · 2021-09-17T20:44:41Z

This committed code implements both TableFactory and Factory. Actually, Flink 1.13 will continue to use TableFactory API. So only implementing TableFactory is the method with the least changes and the lowest risk to upgrade to 1.13. Which way do you prefer? Implementing both or just TableFactory?

Oh that's rather unfortunate. I had hoped that Flink 1.13 would use the new interface, if present, while allowing for the old interface to also be implemented for backwards compatibility.

Perhaps somebody more involved with the Flink Community than myself can comment as to whether or not there might be a flag added or that could be added to tell Flink to prefer the new interface first (when present).

I would suggest implementing both interfaces for now (and also organizing the interface functions so that they're organized together based on which interface), so as to keep support for Flink 1.12.

While this won't give us the benefit of the new API, we can upgrade to Flink 1.13.2 and then consider the importance of that later on. I would still implement the new API functions though (since it's already been done), and then we can track that in a follow up issue. We might indeed decide dropping support for 1.12 is worth it, but we can't make that decision on our own.

EDIT: Perhaps it can be broken up into two different classes, and then when we build the jar only one META-INF file is placed on the class path depending on the Flink version. Seems really messy, but possibly we can consider this down the road.

Any thoughts on this @stevenzwu @openinx?

kbendick · 2021-09-17T20:45:07Z

versions.props

@@ -1,7 +1,7 @@
 org.slf4j:* = 1.7.25
 org.apache.avro:avro = 1.10.1
 org.apache.calcite:* = 1.10.0


Do we know if our calcite version needs to be upgraded?

The apache flink runtime won't depend on this calcite jar , this calcite was introduced for hive before. So I think we don't need to change this calcite version for flink upgrading.

openinx · 2021-09-22T03:36:19Z

api/src/test/java/org/apache/iceberg/AssertHelpers.java

+  public static void assertThrowsRootCause(String message,
+                                       Class<? extends Exception> expected,
+                                       String containedInMessage,
+                                       Runnable runnable) {


Nit: It's not the correct iceberg indent, I think you will need to configure your IDE by following this: https://iceberg.apache.org/community/#setting-up-ide-and-code-style

openinx · 2021-09-22T08:23:31Z

flink/src/main/java/org/apache/iceberg/flink/FlinkCatalogFactory.java

+  }
+
  @Override
  public Map<String, String> requiredContext() {


As the requiredContext and supportedProperties have been marked as deprecated, I will suggest to implement those in requiredOptions & optionalOptions.

As Context class is missing in 1.12, I think we can only implement these deprecated methods.

openinx · 2021-09-22T08:29:51Z

flink/src/main/resources/META-INF/services/org.apache.flink.table.factories.Factory


-org.apache.iceberg.flink.FlinkDynamicTableFactory
+org.apache.iceberg.flink.FlinkDynamicTableFactory
+org.apache.iceberg.flink.FlinkCatalogFactory


Should we remove this file org.apache.flink.table.factories.TableFactory) if we plan to upgrade the flink version to 1.13.2 directly ?

If we only support 1.13, then is ok to remove it. But if we want to be compatible with 1.12, FlinkCatalogFactory need to be treated as TableFactory because Context class is missing in 1.12 and without implementing createCatalog(Context context) will throw exception when we place FlinkCatalogFactory in org.apache.flink.table.factories.Factory

openinx · 2021-09-22T08:40:44Z

@kbendick @Flyangz According to the last discussion from mail list , I think we'd better to keep the same strategy as the apache spark plan to do, users from apache iceberg community also proposed to support both flink 1.12 & flink 1.13. Since the difference between flink 1.12 & 1.13 is not so large, I think it's possible to support both of them in the single flink module ( I mean we don't need to separate them into two different modules).

Flyangz · 2021-09-25T04:42:29Z

In order to be compatible with Flink 1.12 and 1.13, this commit only implements TableFactory interface. By 'compatible', I mean the code can be compiled with both 1.12 and 1.13 and pass all unit tests in iceberg-flink module.
I have tried to make iceberg to use different META-INF file based on Flink version, but this still does not work cause the createCatalog(Context context) method in Factory. The Context object is new in 1.13. Are there any feasible way that we can compile iceberg depended on 1.12 without this class?
@kbendick @openinx

openinx · 2021-09-26T05:07:59Z

In order to be compatible with Flink 1.12 and 1.13, this commit only implements TableFactory interface. By 'compatible', I mean the code can be compiled with both 1.12 and 1.13 and pass all unit tests in iceberg-flink module.
I have tried to make iceberg to use different META-INF file based on Flink version, but this still does not work cause the createCatalog(Context context) method in Factory. The Context object is new in 1.13. Are there any feasible way that we can compile iceberg depended on 1.12 without this class?
@kbendick @openinx

Is the deprecated createCatalog work for both flink 1.12 & flink 1.13 ?

Flyangz · 2021-09-26T06:22:43Z

In order to be compatible with Flink 1.12 and 1.13, this commit only implements TableFactory interface. By 'compatible', I mean the code can be compiled with both 1.12 and 1.13 and pass all unit tests in iceberg-flink module.
I have tried to make iceberg to use different META-INF file based on Flink version, but this still does not work cause the createCatalog(Context context) method in Factory. The Context object is new in 1.13. Are there any feasible way that we can compile iceberg depended on 1.12 without this class?
@kbendick @openinx

Is the deprecated createCatalog work for both flink 1.12 & flink 1.13 ?

I think so, at least it can pass all the unit tests in the iceberg-flink module compiled with 1.12 or 1.13.
In 1.13, as I mentioned above, Flink will use deprecated createCatalog first for compatibility. In 1.12, this method is not deprecated and has already been used in current iceberg master code.

openinx · 2021-09-26T06:57:29Z

Okay, so does any other problems for guaranteeing compatibility for both flink 1.12 & flink 1.13 in your own context ? I think we will need to make the whole unit tests work for both flink 1.12 & flink 1.13 , the work should be similar to this PR: 111fe81. I think it's good to make it into a separate PR !

Flyangz · 2021-09-26T07:33:16Z

Okay, so does any other problems for guaranteeing compatibility for both flink 1.12 & flink 1.13 in your own context ? I think we will need to make the whole unit tests work for both flink 1.12 & flink 1.13 , the work should be similar to this PR: 111fe81. I think it's good to make it into a separate PR !

No.
Yes, integration testing is a more reliable way to check compatibility.

openinx

LGTM ! Thanks @Flyangz for contributing, and thanks @kbendick for reviewing !

openinx · 2021-09-26T08:08:12Z

@Flyangz Please file a new issue to address the unit tests things, and attach the issue to [Priority 1] Flink: Upgrade to 1.13.2 project.

Flyangz · 2021-09-27T08:39:22Z

@Flyangz Please file a new issue to address the unit tests things, and attach the issue to [Priority 1] Flink: Upgrade to 1.13.2 project.

Sorry for the late reply. Do you mean #3183 issue that you have already created ?

openinx · 2021-09-28T02:37:01Z

@Flyangz Yes, this's the correct issue. You can just assign it to yourself.

upgrade to flink 1.13

ff57f9b

github-actions bot added API build flink labels Sep 14, 2021

Flyangz closed this Sep 14, 2021

Flyangz reopened this Sep 14, 2021

Flyangz force-pushed the upgrade-flink1.13 branch from cc38d44 to ff57f9b Compare September 17, 2021 13:48

yintai added 3 commits September 17, 2021 21:49

Merge branch 'master' into upgrade-flink1.13

3437d99

support 1.12 and 1.13

9d1e391

fixed IllegalImport

17dfb6b

kbendick reviewed Sep 17, 2021

View reviewed changes

flink/src/main/java/org/apache/iceberg/flink/FlinkCatalogFactory.java Show resolved Hide resolved

kbendick reviewed Sep 17, 2021

View reviewed changes

Merge branch 'master' into upgrade-flink1.13

d160ad0

openinx changed the title ~~upgrade to flink 1.13~~ Flink: Upgrade to flink 1.13.2 Sep 22, 2021

openinx reviewed Sep 22, 2021

View reviewed changes

yintai added 3 commits September 24, 2021 16:36

Merge branch 'master' into upgrade-flink1.13

ab2f8eb

Merge branch 'master' into upgrade-flink1.13

fb784b5

compatible with 1.12 and 1.13

f94f7ce

openinx approved these changes Sep 26, 2021

View reviewed changes

openinx merged commit bae5ce9 into apache:master Sep 26, 2021

Reo-LEI mentioned this pull request Sep 27, 2021

Flink: Get NoSuchMethodError when submit flink sql by flink 1.12.x since iceberg upgrade flink dependence to 1.13.2 #3187

Closed

This was referenced Oct 19, 2021

Caused by: java.lang.NoClassDefFoundError: org/apache/flink/table/runtime/typeutils/InternalTypeInfo #3096

Closed

Build: Flink CI work against both flink 1.12.5 & 1.13.2 #3364

Merged

ghost mentioned this pull request Nov 1, 2021

[INLONG-1720] Introduce the basic framework for a workflow based on flinksql apache/inlong#1721

Closed

1 task

rpchen mentioned this pull request Jan 5, 2022

release "apache-iceberg-0.12.1" does not contain the commit "Flink: Upgrade to flink 1.13.2 (#3116)" #3847

Closed

donghaihu mentioned this pull request Jan 5, 2022

Flink: upgrade to Flink 1.13.2 #3848

Closed

Flink: Upgrade to flink 1.13.2 #3116

Flink: Upgrade to flink 1.13.2 #3116

Uh oh!

Conversation

Flyangz commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbendick commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wg1026688210 commented Sep 15, 2021

Uh oh!

kbendick commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flyangz commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kbendick commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openinx commented Sep 22, 2021

Uh oh!

Flyangz commented Sep 25, 2021

Uh oh!

openinx commented Sep 26, 2021

Uh oh!

Flyangz commented Sep 26, 2021

Uh oh!

openinx commented Sep 26, 2021

Uh oh!

Flyangz commented Sep 26, 2021

Uh oh!

openinx left a comment

Choose a reason for hiding this comment

Uh oh!

openinx commented Sep 26, 2021

Uh oh!

Flyangz commented Sep 27, 2021

Uh oh!

openinx commented Sep 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Flyangz commented Sep 14, 2021 •

edited

Loading

kbendick commented Sep 14, 2021 •

edited

Loading

kbendick commented Sep 15, 2021 •

edited

Loading

Flyangz commented Sep 17, 2021 •

edited

Loading

kbendick commented Sep 17, 2021 •

edited

Loading