-
Notifications
You must be signed in to change notification settings - Fork 34
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
DBZ-8325 Emit DDL events #215
Conversation
@jpechane new PR for debezium-vitess-connector is ready for review |
…a changes to false
@@ -368,6 +382,14 @@ public static BigIntUnsignedHandlingMode parse(String value, String defaultValue | |||
.withValidation(CommonConnectorConfig::validateTopicName) | |||
.withDescription("Overrides the topic.prefix used for the data change topic."); | |||
|
|||
public static final Field OVERRIDE_SCHEMA_CHANGE_TOPIC = Field.create("override.schema.change.topic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpechane Following up on discussion here, we do still need this config. The SchemaTopicNamingStrategy does not override schemaChangeTopic
so it defaults to the implementation in AbstractTopicNamingStrategy ie just using topic.prefix. We do need a custom config here for vitess-specific use cases. I updated the docstring in the vitess.TableTopicNamingStrategy
to provide more context on this (ie with vitess-connectors we can have mulitple connectors for the same database, so need to be able to override this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You provide io.debezium.connector.vitess.TableTopicNamingStrategy
. So why can't you provide two strategies that would differ in shcema handling? Or if you want to make it configurable, there is a configure
method at teh strategy class level so you can move the config field into the strategy itself in the same way as is done at io.debezium.schema.AbstractTopicNamingStrategy
. That way it will be configurable yet the field will not be defined at connector level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we separate the strategies (one does the override data change topic, one does the schema change topic override) then afaik we can't combine two topic naming strategies in the config so we cannot use both of them. If vitess-connectors want to enable this feature and they have multiple connectors per database they would need to use both (see this test for demonstration).
I will go with that second option of moving the configs into the vitess.TableTopicNamingStrategy class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twthorn When we solve the issue of the topic strategy configuration the PR is good to go. It seems its quite isolated now and feature is disabled by default so I think it can go even into 3.0.x. I plan to do 3.0.2 on Thu/Fri so if we can sort it out it an be merged and released this week.
@twthorn And it is applied! Thanks for the effort and patience, it is very nice result. |
Based on feedback on #210 creating this PR. Depends on debezium/debezium#5979 (so tests won't pass until that is merged) which allows for publishing schema change events without using the Historized configs/schemas. These are not necessary to use for Vitess VStream so it simplifies the code greatly.
Note: although Vitess DDLs use MySQL syntax, there is a requirement to use the MySQL parser that the databaseTables be up to date prior to receiving a DDL event otherwise it is trivially ignored (eg here). Since Vitess VStream may send a DDL as the first event (with no prior FIELD event to establish the table schema), then this is not feasible to know (without doing a full historized snapshot with CREATE table statements). To work around this we use a simple regex and don't bother parsing the full SQL text. This still allows us to set fields correctly (eg
ddl
andtable
field are correct, and we simply leavetableChanges
field empty). In a separate ticket we can change the parser to possibly support this, but that is a larger change/refactor that I believe would best be in a separate PR.Example DDL event: