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

[oceanbase] Add new deserialization schema with runtime converter #1356

Merged
merged 2 commits into from
Oct 19, 2022
Merged

[oceanbase] Add new deserialization schema with runtime converter #1356

merged 2 commits into from
Oct 19, 2022

Conversation

whhe
Copy link
Member

@whhe whhe commented Jul 7, 2022

The previous logic is to do the first conversion with OceanBaseJdbcConverter helper methods, and then use runtime converters in JdbcValueConverters to do the second conversion. Now we need only do conversion one time with the new deserialization schema.

close #980 close #1289

@whhe
Copy link
Member Author

whhe commented Jul 7, 2022

@leonardBang please help to review this pr. The test failure seems not related to my modification, so I will not try to fix it.

@whhe
Copy link
Member Author

whhe commented Jul 21, 2022

Any maintainer can help review this? @GOODBOY008 @Jiabao-Sun

@leonardBang
Copy link
Contributor

@ruanhang1993 Would you like to help review this PR ?

@whhe whhe force-pushed the runtimeConverter branch 2 times, most recently from 9b53329 to 9c7adde Compare August 10, 2022 08:13
@whhe
Copy link
Member Author

whhe commented Aug 11, 2022

There might be something wrong on the self-hosted agent machine, the oceanbase container can't be started now. @leonardBang

@leonardBang
Copy link
Contributor

There might be something wrong on the self-hosted agent machine, the oceanbase container can't be started now. @leonardBang

let me see see

ruanhang1993
ruanhang1993 reviewed Aug 11, 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 work. I have left some comments.
Besides these, I find that there are many duplicated code between the new code and the code in module flink-connector-debezium, e.g. AppendMetadataCollector. Is there a way to reuse them?

...ava/com/ververica/cdc/connectors/oceanbase/source/RowDataOceanBaseDeserializationSchema.java Show resolved Hide resolved
...ava/com/ververica/cdc/connectors/oceanbase/source/RowDataOceanBaseDeserializationSchema.java Show resolved Hide resolved
...ava/com/ververica/cdc/connectors/oceanbase/source/RowDataOceanBaseDeserializationSchema.java Show resolved Hide resolved
...ava/com/ververica/cdc/connectors/oceanbase/source/RowDataOceanBaseDeserializationSchema.java Outdated Show resolved Hide resolved
flink-connector-oceanbase-cdc/src/test/resources/ddl/column_type_test.sql Show resolved Hide resolved
...cdc/src/test/java/com/ververica/cdc/connectors/oceanbase/table/OceanBaseConnectorITCase.java Show resolved Hide resolved
@whhe
Copy link
Member Author

whhe commented Aug 11, 2022

Thanks for your work. I have left some comments. Besides these, I find that there are many duplicated code between the new code and the code in module flink-connector-debezium, e.g. AppendMetadataCollector. Is there a way to reuse them?

It's hard to reuse codes in RowDataDebeziumDeserializeSchema and AppendMetadataCollector in debezium module, because they both are final classes.

@whhe
Copy link
Member Author

whhe commented Aug 15, 2022

@ruanhang1993 Any question for now?

@ruanhang1993
Copy link
Contributor

@ruanhang1993 Any question for now?

No more questions from my side. : )

@whhe
Copy link
Member Author

whhe commented Aug 29, 2022

Any progress for this PR? @leonardBang

@whhe whhe mentioned this pull request Sep 2, 2022
Closed
leonardBang
leonardBang approved these changes Oct 19, 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 @whhe and @ruanhang1993 for the great work, LGTM

@leonardBang leonardBang merged commit b1b092c into apache:master Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanhang1993 ruanhang1993 ruanhang1993 left review comments

@leonardBang leonardBang leonardBang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@whhe @leonardBang @ruanhang1993