-
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 #210
Conversation
There are two integration tests failing due to some issues with parsing 0000-00-00 dates, I will look into these, the rest are passing with these changes. |
All integration tests pass locally with the changes in debezium/debezium#5939 |
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.
Some minor comments.
|
||
@Override | ||
public String schemaChangeTopic() { | ||
String topicName; |
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.
It doesn't seem you are using topicName in the method
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.
Nice catch, updated!
return overrideSchemaChangeTopicName; | ||
} | ||
else { | ||
return prefix; |
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.
What's the value of prefix
in this case?
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.
Good point, updated java doc!
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.
lgtm
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 I created the 3.1 branch so the PR could be redirected to be merged to it.
The PR looks functional but I am afraid I am not really happy about the idea of using database history just to bypass core checks or hook into the code.
I if possible would definitely prefer the changes to the core that would allow the connector without historized schema to publish its schema changes too (PostgreSQL would benefit from it for example). A CommonConnectorConfig
could be for example used for that.
@@ -368,6 +433,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.
Please remove this one, it can be solved using https://debezium.io/documentation/reference/3.0/connectors/vitess.html#vitess-property-topic-naming-strategy
<artifactId>debezium-connector-mysql</artifactId> | ||
<version>${version.debezium}</version> | ||
</dependency> | ||
<dependency> |
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.
IMHO this dependency is not needed as it is a transitive one for debezium-connector-mysql
@jpechane Thank you for the review. I will work on:
As a follow up change (separate than either of these to keep things smaller scoped) we could create another form of the parser that doesn't rely on schema history. |
Closing in favor of |
Add support to emit DDL events.
The only way to emit DDL events currently is with historized connectors/configs/schemas because of this check.
I thought about using the binlog schema/connector but from what I can tell this is not appropriate since we are not actually interacting directly with binlogs (we get protobuf grpc events from Vitess). Although there is overlap, it seemed too much to modify that to fit our use case, so I opted for more general historized versions.
There is no need for internal use of the schema history as we receive a table map (ie field) event before each change. All we really want is a way to be able to publish DDL events for external downstream users to consume (so they can stay up to date on changes that aren't simply DMLs, ie DDLs that change the rows eg truncate tables, drop partition). If there's a simpler way to do this then that would be preferred.
From what I can tell to leverage the existing DDL parsers for MySQL we do need to have the in-memory representation of the tables be up to date otherwise parsing does not generate DDL events. ie on startup we need to do a schema snapshot or have some way of finding the schema of the tables, otherwise it's a race condition with receiving a row change (that includes a table map event) and receiving a DDL event (if DDL received before the row change/table map, it can't be parsed, if received after then it can be parsed). So this seems it does require us to have a full historized schema/connector/config support.
Note: we will refactor the debezium-connector-mysql/binlog so we don't need these, we can move the parsing to another module to be shared. This is an initial PR to get early feedback.
Note: the itests trivially fail here since we made a modification to debezium main repo dependencies. The PR neede for these changes to work is here debezium/debezium#5939. It removes the converters as suggested in this thread