Skip to content

DBZ-4353 Add test for online ddl schema migration#52

Merged
gunnarmorling merged 8 commits into
debezium:mainfrom
5antelope:DBZ-4353/test
Nov 29, 2021
Merged

DBZ-4353 Add test for online ddl schema migration#52
gunnarmorling merged 8 commits into
debezium:mainfrom
5antelope:DBZ-4353/test

Conversation

@5antelope
Copy link
Copy Markdown
Member

@5antelope 5antelope commented Nov 24, 2021

Addressed https://issues.redhat.com/browse/DBZ-4353 to add test cases for online ddl schema migration

A few noticeable changes are:

@5antelope 5antelope force-pushed the DBZ-4353/test branch 4 times, most recently from 2985aee to 71b9a2a Compare November 24, 2021 07:24
@5antelope
Copy link
Copy Markdown
Member Author

I'm getting weird errors complaining d[] != d[]:

VitessConnectorIT.shouldReceiveChangesForInsertsWithDifferentDataTypes:119->assertInsert:590->assertInsert:617->AbstractVitessConnectorTest.assertRecordSchemaAndValues:377->AbstractVitessConnectorTest.lambda$assertRecordSchemaAndValues$0:378 Values don't match for field 'binary_col' expected:<d[]> but was:<d[]>

@keweishang do you have some idea what might be the issue here? thanks!

@5antelope 5antelope force-pushed the DBZ-4353/test branch 2 times, most recently from 560505b to 79db211 Compare November 24, 2021 22:22
@5antelope
Copy link
Copy Markdown
Member Author

I figured it out, the failing test is caused by the padding for binary column introduced at Vitess vitessio/vitess#8137

ptal @shichao-an @jeffchao @gunnarmorling

@jeffchao
Copy link
Copy Markdown
Member

@Sonne5 can you add some more context in the PR description? I'm wondering about the behavior we didn't catch before accounting for the Vitess padding change.

@5antelope
Copy link
Copy Markdown
Member Author

@Sonne5 can you add some more context in the PR description? I'm wondering about the behavior we didn't catch before accounting for the Vitess padding change.

@jeffchao Sure thing. I added some more context in the description. There is no logic change at all, just adding a test case for schema migration. All other code changes are made either for refactoring to reuse or adjust test case for new Vitess's behavior. lmk if it is clear to you.

@jeffchao
Copy link
Copy Markdown
Member

Nice, lgtm.

Copy link
Copy Markdown
Member

@jeffchao jeffchao left a comment

Choose a reason for hiding this comment

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

lgtm, but also my first review in this org, so let's hear from @gunnarmorling (when he's online) to see if he has any thoughts as well.

Copy link
Copy Markdown
Member

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Sonne5; some comments inline. Mostly cosmetics, but I'm not 100% about the testing approach. Is it actually the case that no changes to the connector code itself are needed for this, i.e. were schema changes supported before, and it's solely a matter of adding this test? Do you have a pointer to where in the connector actually handles that?

Comment thread src/test/docker/Dockerfile
Comment thread src/test/java/io/debezium/connector/vitess/TestHelper.java Outdated
Comment thread src/test/java/io/debezium/connector/vitess/TestHelper.java Outdated
Comment thread src/test/java/io/debezium/connector/vitess/VitessSchemaMigrationIT.java Outdated
Comment thread src/test/java/io/debezium/connector/vitess/VitessSchemaMigrationIT.java Outdated
Comment thread src/test/java/io/debezium/connector/vitess/VitessSchemaMigrationIT.java Outdated
Comment thread src/test/java/io/debezium/connector/vitess/VtctldConnection.java Outdated
return null;
}
});
ReplicationMessage dml = message.getMessage();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I'm a bit confused about this test; I would rather have expected a new test method added to VitessConnectorIT, which asserts the actual SourceRecord emitted by the connector, including the schema of the new field. I fear with the currently proposed test we're not making sure that the functionality actually works as intended.

Copy link
Copy Markdown
Member Author

@5antelope 5antelope Nov 25, 2021

Choose a reason for hiding this comment

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

Good point, I added another test case in VitessConnectorIT and also move this one into VitessReplicationConnectionIT since this test case is verifying Vitess returns events as expected - lmk if this makes sense to you :-)

@5antelope
Copy link
Copy Markdown
Member Author

5antelope commented Nov 25, 2021

Is it actually the case that no changes to the connector code itself are needed for this, i.e. were schema changes supported before, and it's solely a matter of adding this test? Do you have a pointer to where in the connector actually handles that?

@gunnarmorling Yes, it was supported before (code pointer). And here is the new test case and verify it works.

When there is a DDL, Vitess will streamed out a FIELD_EVENT, this is the signal for the VStream consumer that they should cache the updated schema. In Debezium case, it is the signal for us to update the schema for the table.

As for test cases, there are multiple approach for DDL in Vitess: we can do "ALTER TABLE" via pure sql protocol or do it through "online ddl". The sql protocol approach works except it is unreliable, given Vitess is a sharded cluster: if the DDL failed on a certain shards, the cluster will be in a weird state and hard to track. Online ddl does is similar to gh-ost but it's using Vitess's own replication logic to copy the temp table, control the cutover, etc - thus in theory it has less production impact comparing to gh-ost or pt-osc.

We have ddl test case for the sql protocol but we didn't have test coverage for online ddl (which is actually the one most ppl are using)

Let me know if this makes sense to you!

Copy link
Copy Markdown
Member

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

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

LGTM, applying. Thx, @Sonne5 et al.!

@gunnarmorling gunnarmorling merged commit e6aebf0 into debezium:main Nov 29, 2021
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.

3 participants