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

[Feature][Core] Support cdc task ddl restore for zeta #7463

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

hailin0
Copy link
Member

@hailin0 hailin0 commented Aug 22, 2024

Purpose of this pull request

[Improve][CDC] Improve cdc task state restore for zeta

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added

https://github.com/apache/seatunnel/pull/7463/files#diff-56c06c7dde242a3733bec78c1d71f5a32baa3530be2c8f336e28de7e390312a8

Check list

@hailin0 hailin0 force-pushed the dev-merge-ddl branch 2 times, most recently from 23d99f9 to 9d7d79f Compare August 22, 2024 12:59
@hailin0 hailin0 marked this pull request as ready for review August 22, 2024 13:33
@dailai dailai mentioned this pull request Aug 23, 2024
3 tasks
Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

It seem lack test case. Shall we add some?
For example, read updated/deleted column data to unsupported transform/sink, what's will happen?
And read table -> savepoint -> update column -> restore? I think the change will solve the problem of this scenario. But we need test case.

import java.util.Collections;
import java.util.List;

public interface ChangeStreamTableSourceFactory extends TableSourceFactory {
Copy link
Member

Choose a reason for hiding this comment

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

What's meaning of ChangeStreamTable? CDC?
Any introduce of why we should add this?

Copy link
Member Author

@hailin0 hailin0 Oct 28, 2024

Choose a reason for hiding this comment

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

e.g. CDC/MQ Source The factory can be used to restore the source state from the checkpoint


import java.io.IOException;

public interface SupportSchemaEvolutionSinkWriter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this interface extend SinkWriter to reduce the amount of interface for the implementing class.

Copy link
Member Author

Choose a reason for hiding this comment

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

DDL should have an independent interface, and the connector can choose to support it, and use test cases to detect whether the user has implemented it correctly.

https://github.com/apache/seatunnel/pull/7463/files/4d062ebcfa9fabf37aff535280874dc1443cb30c#diff-4e38101d35f3d5901ed61ddd52530609bc8facb2d1e41476a981b18b3b2ffb9cR198

Other than that, I don't think of a better way. Using extends classes is more difficult to maintain.


@Override
public List<SchemaChangeType> supports() {
SeaTunnelSink firstSink = sinks.entrySet().iterator().next().getValue();
Copy link
Contributor

@dailai dailai Aug 26, 2024

Choose a reason for hiding this comment

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

The sink action may include different kinds of sink, if the first sink that do not support SupportSchemaEvolutionSink and other support?

Copy link
Member Author

Choose a reason for hiding this comment

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

For MultiTableSink it should only contain sinks of the same kind
cc @Hisoka-X

Copy link
Member

Choose a reason for hiding this comment

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

For MultiTableSink it should only contain sinks of the same kind

Right for now.

@hailin0
Copy link
Member Author

hailin0 commented Aug 27, 2024

It seem lack test case. Shall we add some? For example, read updated/deleted column data to unsupported transform/sink, what's will happen? And read table -> savepoint -> update column -> restore? I think the change will solve the problem of this scenario. But we need test case.

Added

https://github.com/apache/seatunnel/pull/7463/files#diff-56c06c7dde242a3733bec78c1d71f5a32baa3530be2c8f336e28de7e390312a8

@Carl-Zhou-CN
Copy link
Member

@hailin0 hi, I think this part needs a design to help me understand the whole process

@hailin0 hailin0 force-pushed the dev-merge-ddl branch 2 times, most recently from 4ba4c76 to 30c44a9 Compare October 26, 2024 11:58
@hailin0 hailin0 changed the title [Improve][CDC] Improve cdc task state restore for zeta [Improve][CDC] Improve cdc task ddl restore for zeta Oct 28, 2024
@hailin0
Copy link
Member Author

hailin0 commented Oct 28, 2024

@hailin0 hi, I think this part needs a design to help me understand the whole process

cc @Carl-Zhou-CN

#7930

@hailin0 hailin0 changed the title [Improve][CDC] Improve cdc task ddl restore for zeta [Feature][CDC] Support cdc task ddl restore for zeta Oct 28, 2024

@Override
public List<SchemaChangeType> supports() {
SeaTunnelSink firstSink = sinks.entrySet().iterator().next().getValue();
Copy link
Member

Choose a reason for hiding this comment

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

For MultiTableSink it should only contain sinks of the same kind

Right for now.

@Hisoka-X Hisoka-X added this to the 2.3.9 milestone Oct 29, 2024
Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

LGTM. cc @dailai for a look

@hailin0 hailin0 changed the title [Feature][CDC] Support cdc task ddl restore for zeta [Feature][Core] Support cdc task ddl restore for zeta Oct 29, 2024
@Hisoka-X Hisoka-X requested a review from dailai November 4, 2024 03:05
@dailai
Copy link
Contributor

dailai commented Nov 4, 2024

LGTM. cc @dailai for a look

Waiting to merge #7908

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants