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.

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] Upgrade sink connector to new API version #205

Merged

Conversation

michaelkoepf
Copy link
Contributor

@michaelkoepf michaelkoepf commented Dec 17, 2024

Purpose

Linked issue: close #132

Upgrade sink connector from deprecated RichSinkFunction (will be removed in Flink 2.0) to new Sink interface

Tests

  • UT (adapted to new implementation, test case logic unchanged): com.alibaba.fluss.connector.flink.sink.FlinkSinkWriterTest.java
  • IT (unchanged, because external interface did not change): com.alibaba.fluss.connector.flink.sink.FlinkTableSinkITCase.java

API and Format

n/a

Documentation

n/a

@ChaomingZhangCN
Copy link
Contributor

LGTM, cc @wuchong.

@michaelkoepf michaelkoepf changed the title [flink] Upgraded sink connector to new API version [flink] Upgrade sink connector to new API version Dec 17, 2024
Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

Sorry for the late reviewing and thanks @michaelkoepf for the contribution. The changes looks good to me in general. Could you rebase the branch to latest main branch to resolve the conflicts?

@michaelkoepf michaelkoepf force-pushed the flink/connectors-upgrade-to-new-sink-api branch 4 times, most recently from 9c08511 to 2da3bdc Compare February 22, 2025 14:54
@michaelkoepf
Copy link
Contributor Author

@wuchong addressed feedback and rebased, ptal

Copy link
Member

@wuchong wuchong 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 the updating. LGTM.

@wuchong wuchong force-pushed the flink/connectors-upgrade-to-new-sink-api branch from 2da3bdc to 5f93c5f Compare February 25, 2025 08:12
@wuchong
Copy link
Member

wuchong commented Feb 25, 2025

Hi @michaelkoepf , I know why the CI is stuck on FlussDatabaseSyncSourceITCase. It can be easily reproduced in local. The reason is the hard cast here will fail, because we have upgraded from SinkFunctionProvider to SinkV2Provider in this PR, and the cast in this line will fail. So we have to upgrade TestingDatabaseSyncSink to SinkV2 as well. Could you help to fix it?

sinkFunction =
((SinkFunctionProvider)
flinkTableSink.getSinkRuntimeProvider(
new SinkRuntimeProviderContext(false)))
.createSinkFunction();

@michaelkoepf
Copy link
Contributor Author

michaelkoepf commented Feb 25, 2025

@wuchong i will look into the suggested issue.

@michaelkoepf michaelkoepf force-pushed the flink/connectors-upgrade-to-new-sink-api branch from 2132b2a to 2da3bdc Compare February 25, 2025 23:52
- Upgraded sink connector from deprecated RichSinkFunction to new Sink interface
- Adapted corresponding unit test cases

Issue alibaba#132
- Adapted access modifiers
- Consistent naming

Issue alibaba#132
@michaelkoepf michaelkoepf force-pushed the flink/connectors-upgrade-to-new-sink-api branch from 2da3bdc to 6dd724f Compare February 26, 2025 23:05
@michaelkoepf
Copy link
Contributor Author

@wuchong fixed

[INFO] Fluss : ............................................ SUCCESS [  1.906 s]
[INFO] Fluss : Test utils ................................. SUCCESS [  3.650 s]
[INFO] Fluss : Common ..................................... SUCCESS [ 15.666 s]
[INFO] Fluss : Metrics : .................................. SUCCESS [  0.077 s]
[INFO] Fluss : Metrics : Prometheus ....................... SUCCESS [  2.893 s]
[INFO] Fluss : Metrics : JMX .............................. SUCCESS [  2.509 s]
[INFO] Fluss : ProtoGen : ................................. SUCCESS [  0.049 s]
[INFO] Fluss : ProtoGen : Generator ....................... SUCCESS [  0.367 s]
[INFO] Fluss : ProtoGen : Maven Plugin .................... SUCCESS [  4.008 s]
[INFO] Fluss : RPC ........................................ SUCCESS [ 19.378 s]
[INFO] Fluss : Server ..................................... SUCCESS [04:28 min]
[INFO] Fluss : Client ..................................... SUCCESS [04:58 min]
[INFO] Fluss : FileSystems : .............................. SUCCESS [  0.053 s]
[INFO] Fluss : FileSystems : Hadoop FS shaded ............. SUCCESS [  3.810 s]
[INFO] Fluss : FileSystems : Hadoop FS .................... SUCCESS [ 20.107 s]
[INFO] Fluss : FileSystems : OSS FS ....................... SUCCESS [ 11.021 s]
[INFO] Fluss : FileSystems : S3 FS ........................ SUCCESS [  7.628 s]
[INFO] Fluss : Connector : ................................ SUCCESS [  0.062 s]
[INFO] Fluss : Connector : Flink .......................... SUCCESS [06:06 min]
[INFO] Fluss : Lakehouse : ................................ SUCCESS [  0.047 s]
[INFO] Fluss : Lakehouse : Paimon ......................... SUCCESS [03:22 min]
[INFO] Fluss : Lakehouse : CLI ............................ SUCCESS [  7.450 s]
[INFO] Fluss : Dist ....................................... SUCCESS [ 12.843 s]

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

LGTM.

@wuchong wuchong merged commit 0b623e4 into alibaba:main Feb 27, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Use new sink api for Flink connector
3 participants