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-4783: Support for multiple databases and tasks in the SQL Server connector #3261
Conversation
Welcome as a new contributor to Debezium, @morozov. Reviewers, please add missing author name(s) and alias name(s) to the COPYRIGHT.txt and Aliases.txt respectively. |
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.
Thanks a lot, @morozov! A few stylistic comments inline. Still needs to do some testing using Compose (thanks for sharing the instructions for that!).
...ver/src/main/java/io/debezium/connector/sqlserver/SqlServerChangeEventSourceCoordinator.java
Outdated
Show resolved
Hide resolved
...um-connector-sqlserver/src/main/java/io/debezium/connector/sqlserver/SqlServerConnector.java
Show resolved
Hide resolved
...um-connector-sqlserver/src/main/java/io/debezium/connector/sqlserver/SqlServerConnector.java
Outdated
Show resolved
Hide resolved
@@ -90,8 +128,7 @@ protected void validateConnection(Map<String, ConfigValue> configValues, Configu | |||
final SqlServerConnectorConfig sqlServerConfig = new SqlServerConnectorConfig(config); | |||
|
|||
if (Strings.isNullOrEmpty(sqlServerConfig.getDatabaseName())) { | |||
throw new IllegalArgumentException("Either '" + SqlServerConnectorConfig.DATABASE_NAME | |||
+ "' or '" + SqlServerConnectorConfig.DATABASE_NAMES | |||
throw new IllegalArgumentException("Either '" + DATABASE_NAME + "' or '" + DATABASE_NAMES |
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.
Rather a configuration error should be added (see connection validation failure in this method below).
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.
There is actually already validation of both parameters:
- For
DATABASE_NAME
:
Line 235 in 2d14b5b
.withValidation(SqlServerConnectorConfig::validateDatabaseName); - And for
DATABASE_NAMES
:Line 243 in 2d14b5b
.withValidation(SqlServerConnectorConfig::validateDatabaseNames)
So we just need to early return if any of these fields is invalid as it was done prior to #2604:
Lines 82 to 85 in ade15cd
final ConfigValue databaseValue = configValues.get(RelationalDatabaseConnectorConfig.DATABASE_NAME.name()); | |
if (!databaseValue.errorMessages().isEmpty()) { | |
return; | |
} |
|
||
List<String> databaseNames; | ||
|
||
if (multiPartitionMode) { |
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.
See above.
…connector Co-authored-by: Sergei Morozov <morozov@tut.by>
@gunnarmorling I addressed your review comments and added a basic snapshotting/streaming test. Please take a look. |
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, merging.
@morozov, is there anything else missing before we can declare multi-partitioning mode as functional? I.e. could we announce it as a new feature in the Beta1 release with this PR in?
There are a couple of minor things left:
And this is it, there are no more changes related to the multi-partition mode in our fork. I will file Jira cases for each of them but I believe it shouldn't block the announcement. |
Thx a lot for logging these follow-up, issues, @morozov! Also a big thank you for that awesome PR description, it just came in super-handy for creating a screenshot of the metrics in JMC. |
Change summary:
Note, running multiple tasks is not necessary for capturing multiple databases. Furthermore, running multiple tasks cannot be tested in the current test suite. The only reason why it's contributed here is that extracting the support for multiple tasks in a separate patch would require extra work (it was originally implemented like this). I can remove it from the patch, if necessary.
TODO:
Manual testing of the multi-database and multi-task configuration
Modify
docker-compose-sqlserver.yaml
from the tutorial to use the Docker image built from this PR.Start the services:
export DEBEZIUM_VERSION=1.9 docker-compose -f docker-compose-sqlserver.yaml up
Initialize test databases:
Start the connector:
Check that the connector and both tasks are running:
Check that each task exposes metrics for its subset of databases:
Run a consumer and confirm that both databases were snapshotted:
Make changes in both databases and confirm that the changes from both databases are captured: