-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
[FLINK-35415][base] Fix compatibility with Flink 1.19 #3348
Conversation
@lvyanquan PTAL |
...ris/src/test/java/org/apache/flink/cdc/connectors/doris/sink/DorisMetadataApplierITCase.java
Show resolved
Hide resolved
// during Flink 1.18 to 1.19. Remove this when Flink 1.18 is no longer supported. | ||
try { | ||
return (SimpleVersionedSerializer<CommT>) | ||
sink.getClass().getDeclaredMethod("getCommittableSerializer").invoke(sink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that we have had similar operations before in DataSinkWriterOperator, this change is acceptable for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it's definitely not an encouraged way to play with Flink :( Maybe we can decouple Flink base with specific connectors and distribute CDC with various Flink versions respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, distribute CDC with various Flink versions in the future is recommended just like other flink external connectors
Thanks for @lvyanquan's review, I've split out commits based on FLINK ticket. |
Pushed another commit to resolve CI issue, could @leonardBang please re-trigger the CI? Thanks! |
...ris/src/test/java/org/apache/flink/cdc/connectors/doris/sink/DorisMetadataApplierITCase.java
Show resolved
Hide resolved
...test/java/org/apache/flink/cdc/connectors/starrocks/sink/StarRocksMetadataApplierITCase.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yuxiqian for the nice work, the change looks good to me. I left minor comments. It will better if you can organize the commits to make each main change more clear e.g. bump flink version could be in one separate commit, fix sink compatibility could be in another commit, and add e2e case could be in another commit.
...omposer/src/main/java/org/apache/flink/cdc/composer/flink/translator/DataSinkTranslator.java
Outdated
Show resolved
Hide resolved
// during Flink 1.18 to 1.19. Remove this when Flink 1.18 is no longer supported. | ||
try { | ||
return (SimpleVersionedSerializer<CommT>) | ||
sink.getClass().getDeclaredMethod("getCommittableSerializer").invoke(sink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, distribute CDC with various Flink versions in the future is recommended just like other flink external connectors
...omposer/src/main/java/org/apache/flink/cdc/composer/flink/translator/DataSinkTranslator.java
Outdated
Show resolved
Hide resolved
Done, seems I messed up the commit history during last rebase... Fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yuxiqian for the update, LGTM
This closes FLINK-35092, FLINK-35120, FLINK-35316, FLINK-35415.
This is a temporary workaround to avoid breaking compatibility with Flink 1.19. Proposal about upgrading Flink base (FLINK-35385) could be implemented later.