DBZ-6731 Blocking snapshot takes configuration from signal payload

[mfvitale]

closes: https://issues.redhat.com/browse/DBZ-6731

[jpechane]

@mfvitale Why is this change needed?

[mfvitale]

Just removed the Optional as parameters since it's not a good practice. Now, in case of missing value is simply empty ""

[jpechane]

I don't think I agree on that one. We use it in the codebase elsewhere as well. The Optional is a way how to conway the message in code that value might be present. Even if originally it was told that Optional should be mostly return value there are differing opinions on that rule too.

[mfvitale]

The main "issue" (it's more about clean code) with Optional in the parameter is that it causes conditional logic inside methods, like passing boolean. This means that the method will have two different flow inside and so doing too much things. Leveraging on method overloading or method with different name can be better.

But it is just a different opinion on that. We can discuss it apart.

[jpechane]

In these cases the Optional is valid even in the strict sense

[jpechane]

Please use better name as Mongo does not use neither select nor tables.

[mfvitale]

Fortunately, your comment revealed a missing. I was convinced that additional conditions was not supported at all but this is not the case. It is unsupported for incremental snapshot but is supported for initial snapshot.

[jpechane]

TableId is relational, it should not leak into MongoDB.

[jpechane]

Doe it mean that the user cannot prescribe order in which tables will be snapshotted?

[mfvitale]

Seems there was different behavior:

• initial snapshot: the snapshot.include.collection.list is converted to a Set<Pattern> so the order is no preserved and the order is eventually controlled by snapshot.tables.order.by.row.count property. In this case the Set prevent same table to be scanned multiple times (for wrong patterns).

• Incremental snapshot: the data-collections property is a list of string (but they are patterns), they are expanded in order. For wrong pattern this can lead to snapshot a table multiple times.

Is it correct that we want to maintain this behaviors?

[jpechane]

Could you please dig a bit deeper into initial snapshot? It seems to me that the order is driven by the table.include.list property.
For the increment snapshot - yes deduplication of matchin tables should be done - could you please create a se parate Jira for it?

[mfvitale]

Could you please dig a bit deeper into initial snapshot? It seems to me that the order is driven by the table.include.list property.

private void determineCapturedTables(RelationalSnapshotContext<P, O> ctx, Set<Pattern> dataCollectionsToBeSnapshotted) throws Exception {
        Set<TableId> allTableIds = getAllTableIds(ctx);
        Set<TableId> snapshottedTableIds = determineDataCollectionsToBeSnapshotted(allTableIds, dataCollectionsToBeSnapshotted).collect(Collectors.toSet());

        Set<TableId> capturedTables = new HashSet<>();
        Set<TableId> capturedSchemaTables = new HashSet<>();

        for (TableId tableId : allTableIds) {
            if (connectorConfig.getTableFilters().eligibleForSchemaDataCollectionFilter().isIncluded(tableId)) {
                LOGGER.info("Adding table {} to the list of capture schema tables", tableId);
                capturedSchemaTables.add(tableId);
            }
        }

        for (TableId tableId : snapshottedTableIds) {
            if (connectorConfig.getTableFilters().dataCollectionFilter().isIncluded(tableId)) {
                LOGGER.trace("Adding table {} to the list of captured tables for which the data will be snapshotted", tableId);
                capturedTables.add(tableId);
            }
            else {
                LOGGER.trace("Ignoring table {} for data snapshotting as it's not included in the filter configuration", tableId);
            }
        }

        ctx.capturedTables = addSignalingCollectionAndSort(capturedTables);
        ctx.capturedSchemaTables = capturedSchemaTables
                .stream()
                .sorted()
                .collect(Collectors.toCollection(LinkedHashSet::new));
    }

All tables are read from the database and then filtered by the pattern specified in the snapshot.include.collection.list properties, if provided. Otherwise all tables will be returned. After that, only tables that are included in table.include.list will be used for the snapshot.

For the increment snapshot - yes deduplication of matchin tables should be done - could you please create a se parate Jira for it?

https://issues.redhat.com/browse/DBZ-6787

[jpechane]

@mfvitale Overall LGTM. My main comments are

• Removal of Optional - we can discuss it later if'd wish but IMHO it is out of scope of this PR and make it quite hard to review. If we decide to drop it then it should be a separate PR
• MongoDB - it seems that relational aspects are leaking to it
• Plase make sure the user can prescribe the order of tables for snapshot

[jpechane]

@mfvitale Applied, great work, thanks!