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

[common] Add Flink 1.15 support. #1504

Merged
merged 4 commits into from
Oct 27, 2022
Merged

[common] Add Flink 1.15 support. #1504

merged 4 commits into from
Oct 27, 2022

Conversation

tigrulya-exe
Copy link
Contributor

Bumped Flink version to 1.15.1.
Added Maven profile for Flink 1.14 compatibility.
Relates to #1363

@tigrulya-exe tigrulya-exe mentioned this pull request Aug 24, 2022
Closed
@tigrulya-exe
Copy link
Contributor Author

I've tried to reproduce oceanbase test failure locally and found out this issue: testcontainers/testcontainers-java#5151. In OceanBaseTestBase we create two containers with host network mode and expose ports, like in issue above and Flink 1.15.1 uses version 1.6.2 of TestContainers. I'll try to fix it

@tigrulya-exe
Copy link
Contributor Author

tigrulya-exe commented Aug 25, 2022 •
edited

The cause of failing my-sql tests can be https://issues.apache.org/jira/browse/FLINK-28861. I've debugged MySqlSourceITCase with Flink 1.15.1 and can acknowledge, that OperatorIDs of same operators were different before and after the savepoint. Bumped Flink version to 1.5.2.

@tigrulya-exe tigrulya-exe force-pushed the feature/flink-1.15-support branch 3 times, most recently from d4979f3 to f9dc004 Compare August 26, 2022 05:09
ruanhang1993
ruanhang1993 reviewed Sep 2, 2022
View reviewed changes
Copy link
Contributor

@ruanhang1993 ruanhang1993 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I left some comments.

...main/java/com/ververica/cdc/connectors/mysql/table/MySqlDeserializationConverterFactory.java Show resolved Hide resolved
...main/java/com/ververica/cdc/connectors/mysql/table/MySqlDeserializationConverterFactory.java Show resolved Hide resolved
...r-mysql-cdc/src/test/java/com/ververica/cdc/connectors/mysql/table/MySqlConnectorITCase.java Show resolved Hide resolved
@tigrulya-exe
Copy link
Contributor Author

tigrulya-exe commented Sep 12, 2022 •
edited

Hi, @ruanhang1993 ! Thanks for comments! I've answered on them, can you recheck PR, please?

ruanhang1993
ruanhang1993 approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@ruanhang1993 ruanhang1993 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution.

snuyanzin
snuyanzin reviewed Oct 11, 2022
View reviewed changes
pom.xml Outdated
<profile>
<id>flink-1.14</id>
<properties>
<flink.version>1.14.4</flink.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it required to still have 1.14.x support?
Why just not to update to 1.15.x?
Or otherwise somehow need to guarantee that future changes will not add some 1.15.x specific api which could break 1.14.x usage... It seems currently ci does not check this profile... or did I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your option, @snuyanzin. I have removed this profile to have a try.

@ruanhang1993
Copy link
Contributor

@tigrulya-exe I think we should add the test environment in cdc e2e tests to verify the flink version 1.15.

leonardBang
leonardBang reviewed Oct 26, 2022
View reviewed changes
pom.xml Outdated
@@ -71,7 +71,8 @@ under the License.
</distributionManagement>

<properties>
<flink.version>1.14.4</flink.version>
<flink.version>1.15.2</flink.version>
<flink.submodule.postfix></flink.submodule.postfix>
Copy link
Contributor

Choose a reason for hiding this comment

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

The property is empty?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the flink 1.15.2 does not need the scala suffix. I will try to remove this config and 1.14 profile.

pom.xml Outdated
@@ -467,6 +474,14 @@ under the License.
</build>

<profiles>
<profile>
<id>flink-1.14</id>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need this @ruanhang1993 , Could we build a connector base on Flink 1.15 and then run it on Flink 1.14 cluster？ I think this is what we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to remove 1.14 profile.

leonardBang
leonardBang approved these changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

Thanks @tigrulya-exe and @ruanhang1993 for the contribution, LGTM

@leonardBang leonardBang merged commit 5b3dd98 into apache:master Oct 27, 2022
@CaoYunzhou
Copy link

when support cdc to elasticsearch 8.x for flink cdc ?

@leonardBang
Copy link
Contributor

when support cdc to elasticsearch 8.x for flink cdc ?

You can open a issue firstly @CaoYunzhou

@tigrulya-exe
Copy link
Contributor Author

Hello, @ruanhang1993 ! Sorry for late response, but why did we remove maven module for flink 1.14 version? As far as I remember you said in #1363, that we need to support multiple Flink versions - 1.13, 1.14 and 1.15. For this purpose I've added module, because we can't just use new version of cdc with older Flink distributions due to incompatible changes in 1.15.

@ruanhang1993
Copy link
Contributor

ruanhang1993 commented Nov 21, 2022 •
edited

Hi, @tigrulya-exe . Sorry for the late response.
Actually we do not need to use the Flink 1.14 in the package phase in order to support the Flink version 1.14.
What we should do is to make sure that cdc connectors compiled by Flink 1.15 could run right in the Flink 1.14/1.13 environment. So the changes you provided in the MySqlDeserializationConverterFactory.java is needed. The API is not contained in Flink 1.15. We provide a new method that can be used in Flink 1.13, 1.14 and 1.15, which provides the ability to support those Flink versions. In other words, we only need to make sure that the APIs and classes we used could be found among multiple Flink versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snuyanzin snuyanzin snuyanzin left review comments

@ruanhang1993 ruanhang1993 ruanhang1993 approved these changes

@leonardBang leonardBang leonardBang approved these changes

@PatrickRen PatrickRen Awaiting requested review from PatrickRen

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@tigrulya-exe @ruanhang1993 @CaoYunzhou @leonardBang @snuyanzin