Skip to content
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-3429 SQL Server properties with spaces #2310

Closed
wants to merge 1 commit into from
Closed

DBZ-3429 SQL Server properties with spaces #2310

wants to merge 1 commit into from

Conversation

kyleyj
Copy link
Contributor

@kyleyj kyleyj commented Apr 14, 2021

SQL Server properties issues with spaces. JIRA Ticket: https://issues.redhat.com/browse/DBZ-3429

@gunnarmorling
Copy link
Member

Thx, @kyleyj! The change looks good, could you only add a test to one of the connectors, so to spot/avoid any future regressions here. Thanks again!

@gunnarmorling
Copy link
Member

Thinking a bit more about this, there is Configuration.getStrings(String, String), which should be invoked. And we may even consider adding Configuration.getTrimmedStrings(String) which specializes on getting trimmed strings from a comma-separated list value. This method could then be used later on for retrieving all those similar options from the configuration.

@gunnarmorling
Copy link
Member

Hey @kyleyj, are you planning to do the changes suggested above? No problem if not, just let us know. Thx!

@kyleyj
Copy link
Contributor Author

kyleyj commented Apr 20, 2021

I've made the following change to use the Configuration.getStrings(String, String). It is still dependent on providing the appropriate regex to remove the spaces. However, I don't see a way to add this new commit to the same PR. Is that possible, or do I just need to close this PR and create a new one?

public Map<TableId, String> getSnapshotSelectOverridesByTable() {
List tableValues = getConfig().getStrings(SNAPSHOT_SELECT_STATEMENT_OVERRIDES_BY_TABLE, "\s*,\s*");

    if (tableValues == null) {
        return Collections.emptyMap();
    }

    Map<TableId, String> snapshotSelectOverridesByTable = new HashMap<>();

    for (String table : tableValues) {
        snapshotSelectOverridesByTable.put(
                TableId.parse(table),
                getConfig().getString(SNAPSHOT_SELECT_STATEMENT_OVERRIDES_BY_TABLE + "." + table));
    }

    return Collections.unmodifiableMap(snapshotSelectOverridesByTable);
}

@gunnarmorling
Copy link
Member

You just can keep pushing further commits to this PR's branch.

@kyleyj
Copy link
Contributor Author

kyleyj commented Apr 21, 2021

I had some git trouble in pushing the additional commit to this same PR. Rather than taking more time to figure it out, I'm going to close this PR and open a new one with the latest commit. New PR is #2320

@kyleyj kyleyj closed this Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants