DBZ-302 Add support for `database.whitelist` mongodb connector config… #253

ekreiser · 2017-07-12T20:39:36Z

DBZ-302 Add support for database.whitelist mongodb connector configuration option

…uration option

rhauch

Thanks for the contribution! This is really nice, but I did note a few small changes. Otherwise, looks great!

rhauch · 2017-07-13T01:04:34Z

debezium-connector-mongodb/src/main/java/io/debezium/connector/mongodb/Replicator.java

@@ -250,7 +252,7 @@ protected boolean performInitialSync() {
        // We need to copy each collection, so put the collection IDs into a queue ...
        final List<CollectionId> collections = new ArrayList<>();
        primaryClient.collections().forEach(id -> {
-            if (collectionFilter.test(id)) collections.add(id);
+            if (databaseFilter.test(id.dbName()) && collectionFilter.test(id))collections.add(id);


Minor nit: watch formatting and the space after the if condition.

space added

rhauch · 2017-07-13T01:05:06Z

debezium-connector-mongodb/src/main/java/io/debezium/connector/mongodb/Replicator.java

@@ -430,6 +432,7 @@ protected boolean handleOplogEvent(ServerAddress primaryAddress, Document event)
                    logger.error("Get current primary executeBlocking", e);
                }
                ServerAddress serverAddress = address.get();
+


While you're making changes, it'd be great to avoid unrelated changes like this.

extra line removed

…uration option

gunnarmorling

Hey @ekreiser, thanks a lot! Looks good to me overall, I've added some comments inline.

gunnarmorling · 2017-07-14T08:39:06Z

...um-connector-mongodb/src/main/java/io/debezium/connector/mongodb/MongoDbConnectorConfig.java

@@ -162,6 +162,18 @@
                                                   .withDependents(COLLECTION_LIST_NAME)
                                                   .withDescription("The databases for which changes are to be captured");

+        /**
+     * A comma-separated list of regular expressions that match the fully-qualified namespaces of collections to be monitored.


This comment seems like a Copy & Paste glitch, I don't think it should mention collections here.

Corrected the comment

gunnarmorling · 2017-07-14T08:39:38Z

...um-connector-mongodb/src/main/java/io/debezium/connector/mongodb/MongoDbConnectorConfig.java

+     * Fully-qualified namespaces for collections are of the form {@code '<databaseName>.<collectionName>'}.
+     * May not be used with {@link #COLLECTION_BLACKLIST}.
+     */
+    public static final Field DATABASE_WHITELIST = Field.create("database.whitelist")


It would be nice to have a database.blacklist, too, so to resemble the collection-level options.

Added database.blacklist support

gunnarmorling · 2017-07-14T08:44:02Z

debezium-connector-mongodb/src/main/java/io/debezium/connector/mongodb/Filters.java

+        if (dbWhitelist != null && !dbWhitelist.trim().isEmpty()){
+            databaseFilter = Predicates.includes(dbWhitelist);
+        } else {
+            databaseFilter = Predicates.includes(".*");


Can we pass a simple predicate which always returns true instead of passing it through the regex engine?

changed to use simple predicate when a pattern isn't set

…uration option as a companion to `database.whitelist`

jpechane · 2017-07-17T07:15:37Z

debezium-connector-mongodb/src/main/java/io/debezium/connector/mongodb/Filters.java


    /**
     * Create an instance of the filters.
     * 
     * @param config the configuration; may not be null
     */
    public Filters(Configuration config) {
+        String dbWhitelist = config.getString(MongoDbConnectorConfig.DATABASE_WHITELIST);
+        String dbBlacklist = config.getString(MongoDbConnectorConfig.DATABASE_BLACKLIST);
+        if (dbWhitelist != null && !dbWhitelist.trim().isEmpty()){


I am sorry but it seems that space is missing between rightmost parenthesis and the left curl brace following

jpechane · 2017-07-17T07:21:39Z

...um-connector-mongodb/src/main/java/io/debezium/connector/mongodb/MongoDbConnectorConfig.java

@@ -252,4 +279,14 @@ private static int validateCollectionBlacklist(Configuration config, Field field
        }
        return 0;
    }
+
+    private static int validateDatabaseBlacklist(Configuration config, Field field, ValidationOutput problems) {


It seems we have an interesting inconsistency here - MySQL connector supports adding both blacklist and whitelist but PostgreSQL requires setting of only one of those options. I'd vote going MySQL way and retrofit the functionality into Postgres connector too. Let Gunnar decide.

I see your point but was trying to be consistent within the mongo connector. The collection.whitelist and collection.blacklist are currently coded to be mutually exclusive (not sure why). If the current collection setting were not coded this way, I would be able to combine collection.whitelist and collection.blacklist patterns that accomplish what I need without ever needing the settings specifically at the database level (possible since the collection comparisons are performed while including the database name as a prefix).

So it turns out, combining includes and exludes aren't supported in the MySQL connector atm. either. Apparently it was meant to be supported at some point (there's some code in place for merging them), but then this validation was added (which you added to MongoDB, too) which prevents giving both options. So I'd suggest we do this for now (i.e. your PR is good as is), and then, as a follow-up task, we allow merging includes + excludes across all the connectors. WDYT?

There's https://issues.jboss.org/browse/DBZ-307 for the follow-up task.

I think it is a good plan to separate this issue out and look at it holistically cross all connectors.

…uration option as a companion to `database.whitelist - fix formatting issue

gunnarmorling · 2017-07-18T07:26:03Z

...um-connector-mongodb/src/main/java/io/debezium/connector/mongodb/MongoDbConnectorConfig.java


    protected static Field.Set EXPOSED_FIELDS = ALL_FIELDS;

    protected static ConfigDef configDef() {
        ConfigDef config = new ConfigDef();
        Field.group(config, "MongoDB", HOSTS, USER, PASSWORD, LOGICAL_NAME, CONNECT_BACKOFF_INITIAL_DELAY_MS,
                    CONNECT_BACKOFF_MAX_DELAY_MS, MAX_FAILED_CONNECTIONS, AUTO_DISCOVER_MEMBERS);
-        Field.group(config, "Events", DATABASE_LIST, COLLECTION_WHITELIST, COLLECTION_BLACKLIST);
+        Field.group(config, "Events", DATABASE_LIST, DATABASE_WHITELIST, DATABASE_BLACKLIST, COLLECTION_WHITELIST, COLLECTION_BLACKLIST);


Hum, so that's a bit weird, there already was DATABASE_LIST before, but just its definition, I can't seem to find code that would read that property, also it's not part of the docs. Seems like an uncompleted attempt of adding an option for specifying the DBs. So I'd suggest to remove this with this PR as well, as it's superseded by the new options now.

gunnarmorling · 2017-07-18T07:27:26Z

Hey @ekreiser, so I think the PR s good, apart from my last comment about removing that left-over DATABASE_LIST option. Could you also send in a PR for updating the docs to cover the new options, too?

Thanks!

…s now superseded by `database.whitelist` and `database.blacklist`

ekreiser · 2017-07-18T14:10:50Z

I added DBZ-309 to track the task of updating the documentation to include the new configuration settings included by this PR.

gunnarmorling · 2017-07-25T07:51:49Z

Rebased and applied. Thanks a lot, @ekreiser! I took the liberty to squash some of the commits and remove some trailing whitespace inserted accidentally. I'll send a PR for updating the docs, too.

DBZ-302 Add support for database.whitelist mongodb connector config…

cff3e10

…uration option

rhauch suggested changes Jul 13, 2017

View reviewed changes

ekreiser added 4 commits July 13, 2017 14:20

DBZ-302 Add support for database.whitelist mongodb connector config…

90206a1

…uration option

DBZ-302 Add support for database.whitelist mongodb connector config…

40d4d6c

…uration option

DBZ-302 Add support for database.whitelist mongodb connector config…

3e5a41b

…uration option

DBZ-302 Add support for database.whitelist mongodb connector config…

97139ef

…uration option

gunnarmorling reviewed Jul 14, 2017

View reviewed changes

DBZ-302 Add support for database.blacklist mongodb connector config…

dfdc1fa

…uration option as a companion to `database.whitelist`

jpechane reviewed Jul 17, 2017

View reviewed changes

DBZ-302 Add support for database.blacklist mongodb connector config…

33e7fef

…uration option as a companion to `database.whitelist - fix formatting issue

gunnarmorling reviewed Jul 18, 2017

View reviewed changes

DBZ-302 Remove database.list configuration since it is unused and i…

8c028ce

…s now superseded by `database.whitelist` and `database.blacklist`

gunnarmorling closed this Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBZ-302 Add support for `database.whitelist` mongodb connector config… #253

DBZ-302 Add support for `database.whitelist` mongodb connector config… #253

ekreiser commented Jul 12, 2017

rhauch left a comment

rhauch Jul 13, 2017

ekreiser Jul 14, 2017

rhauch Jul 13, 2017

ekreiser Jul 14, 2017

gunnarmorling left a comment

gunnarmorling Jul 14, 2017

ekreiser Jul 14, 2017

gunnarmorling Jul 14, 2017

ekreiser Jul 14, 2017

gunnarmorling Jul 14, 2017

ekreiser Jul 14, 2017

jpechane Jul 17, 2017

ekreiser Jul 17, 2017

jpechane Jul 17, 2017

ekreiser Jul 17, 2017

gunnarmorling Jul 18, 2017

gunnarmorling Jul 18, 2017

ekreiser Jul 18, 2017

gunnarmorling Jul 18, 2017

gunnarmorling commented Jul 18, 2017

ekreiser commented Jul 18, 2017

gunnarmorling commented Jul 25, 2017

DBZ-302 Add support for database.whitelist mongodb connector config… #253

DBZ-302 Add support for database.whitelist mongodb connector config… #253

Conversation

ekreiser commented Jul 12, 2017

rhauch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunnarmorling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunnarmorling commented Jul 18, 2017

ekreiser commented Jul 18, 2017

gunnarmorling commented Jul 25, 2017

DBZ-302 Add support for `database.whitelist` mongodb connector config… #253

DBZ-302 Add support for `database.whitelist` mongodb connector config… #253