-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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-6709 STORE_ONLY_CAPTURED_DATABASES_DDL should default to false #4875
base: main
Are you sure you want to change the base?
Conversation
@ani-sha Thanks, it seems the test failures are related. |
@jpechane working on the test! |
5560871
to
72284ad
Compare
Hi @ani-sha, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
@Naros Could you please take a look on why the snapshot tests are failing? I have fixed the remaining tests. Thanks. |
Hi @ani-sha, so this is somewhat of a nasty bug with how we handle default values and resolving of those values with the The two test failures before the PR had the following state set:
So essentially the schema changes that were emitted were filtered. With the PR changes, we see the following:
When the key is set to a non-default value, the outcome is correct; however, the issue is when the configuration property is set to its default or not provided, there was a deviation in behavior. By aligning the default value for MySQL with that of the super-type, we should be back to a common slate. I would suggest the following changes:
@jpechane would you concur? |
Hi, in fact the option 2 makes sense as now the default is the same as the referred field so it is completely suprefluous. I'd also recommend to check description of |
@jpechane seeing the description of |
Why do we have this separate config |
Hi @ani-sha, the main reason was that the initial implementation of this option was that for non-MYSQL we would always capture all database-related DDL changes by default, but for MYSQL we limited this to only the captured databases likely due to the potential volume of changes that could be captured with MYSQL supporting multiple independent databases by instance install. Now that we've unified the behavior of MYSQL with the other relational connectors, the MYSQL-specific setting no longer makes sense. |
@@ -213,7 +213,7 @@ else if (Objects.equals(previousSnapshotSourceField, "last_in_data_collection")) | |||
.isEqualTo(schemaEventsCount); | |||
assertThat(schemaChanges.ddlRecordsForDatabaseOrEmpty("").size() | |||
+ schemaChanges.ddlRecordsForDatabaseOrEmpty(OTHER_DATABASE.getDatabaseName()).size()) | |||
.isEqualTo(useGlobalLock ? 1 : 5); | |||
.isEqualTo(1); |
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.
@ani-sha Have you tried to investigate why the change is necessary? Why the test is not behaving in the same way as before?
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.
No @jpechane, I haven't checked that. Let me try it.
https://issues.redhat.com/browse/DBZ-6709