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-3271 Unifying filter handling across relational connectors #2211

Merged
merged 2 commits into from Mar 17, 2021

Conversation

gunnarmorling
Copy link
Member

@gunnarmorling gunnarmorling commented Mar 12, 2021

@gunnarmorling
Copy link
Member Author

@jpechane, @Naros, could either of you review and merge this PRs and its siblings for Vitess and Db2? Thx!

*
* @author Gunnar Morling
*/
public enum ColumnFilterMode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rrename this class and generalize it? I can envision it will not be used only for column filters but another use case too - e.g. https://issues.redhat.com/browse/DBZ-3244

Also could you add one more mode - FULL that would requrie all three components? Albeit not needed now with multiple datbases support it will be the naturally needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do those changes when we actually need them? I'm a bit reluctant to build something "just in case", without knowing what actually is required. We can easily refactor this once the demand is there.

private final TemporalPrecisionMode temporalPrecisionMode;
private final KeyMapper keyMapper;
private final TableIdToStringMapper tableIdMapper;

protected RelationalDatabaseConnectorConfig(Configuration config, String logicalName, TableFilter systemTablesFilter,
TableIdToStringMapper tableIdMapper, int defaultSnapshotFetchSize) {
TableIdToStringMapper tableIdMapper, int defaultSnapshotFetchSize,
ColumnFilterMode columnFilterMode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we are proliferating consturctor params too much. Can't the connector expose a method like columnFilterMode() so it wouldn't be necessary to pass the param? Also SCHEMA will be the default one and CATALOG will be override for two connectors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, hum. I'm not a fan of the method approach, as it's another method exposed on the API surface of RelationalDatabaseConnectorConfig and could be called by other classes (which it shouldn't). How about I add another constructor without the parameter, which does the defaulting to SCHEMA?

@jpechane jpechane merged commit 1f02d25 into debezium:master Mar 17, 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