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

Support metadata columns for mysql-cdc connector #496

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

wuchong
Copy link
Member

@wuchong wuchong commented Oct 11, 2021

We introduced following metadata columns in this PR:

Key DataType Description
database_name STRING NOT NULL Name of the database that contain the row.
table_name STRING NOT NULL
Name of the table that contain the row.
op_ts TIMESTAMP_LTZ(3) NOT NULL It indicates the time that the change was made in the database. If the record is read from snapshot of the table instead of the binlog, the value is always 0.


// TODO: we execute an additional DELETE statement to make incremental-snapshot
// mode pass, in that mode, the first statement will be ignored which should be a bug!
statement.execute("DELETE FROM shopping_cart WHERE product_no=101;");
Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a bug in new source, we need to investigate in another issue.

@leonardBang
Copy link
Contributor

The tests failed

@ashulin
Copy link
Member

ashulin commented Oct 12, 2021

I did a review of your code, and I think your implementation is better.

Comment on lines 216 to 224
/** {@link SourceRecord} metadata info converter. */
@FunctionalInterface
public interface MetadataConverter extends Serializable {
Object read(SourceRecord record);
}

/** Emits a row with physical fields and metadata fields. */
private static final class OutputProjectionCollector
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 possible to define this MetadataConverter as a generic interface and separate MetadataConverter and OutputProjectionCollector from RowDataDebeziumDeserializeSchema so that other connectors can reuse this logic.

@@ -101,12 +101,12 @@ public void setup() throws Exception {
}

@Test
public void testMysqlServerInBerlin() throws Exception {
public void testMySqlServerInBerlin() throws Exception {
testTemporalTypesWithMySqlServerTimezone("Asia/Shanghai");

Choose a reason for hiding this comment

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

testTemporalTypesWithMySqlServerTimezone("Asia/Shanghai") -> testTemporalTypesWithMySqlServerTimezone("Europe/Berlin")

@XuQianJin-Stars
Copy link

LGTM, When the CI Pass.

@@ -622,7 +726,7 @@ public void testPrimaryKeyWithSnowflakeAlgorithm() throws Exception {
}

@Test
public void testInconsistentSchema() throws Exception {
public void testShadingTablesWithInconsistentSchema() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public void testShadingTablesWithInconsistentSchema() throws Exception {
public void testShardingTablesWithInconsistentSchema() throws Exception {

}
}),

TIMESTAMP(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a description for every metadata? And it's better to add the feature in document.

@@ -479,6 +483,106 @@ public void testWideTable() throws Exception {
result.getJobClient().get().cancel().get();
}

@Test
public void testMetadataColumns() throws Exception {
userDatabase1.createAndInitialize();
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to cleanup the userDatabase1 resource, otherwise the test testShadingTablesWithInconsistentSchema will fail due to matching the database creating in this test.

@wuchong
Copy link
Member Author

wuchong commented Oct 12, 2021

Thanks all for the feedbacks, I have address all your comments.

@Jiabao-Sun
Copy link
Contributor

Thanks @wuchong, I'll open a PR to support mongodb metadata columns as well.

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.

LGTM, Thanks @wuchong and @ashulin for the great work

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] Expose metadata columns for mysql-cdc connector
5 participants