lvyanquan commented on code in PR #4362:
URL: https://github.com/apache/flink-cdc/pull/4362#discussion_r4439376072
##########
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-postgres-cdc/src/main/java/org/apache/flink/cdc/connectors/postgres/table/PostgreSQLTableFactory.java:
##########
@@ -127,6 +128,7 @@ public DynamicTableSource
createDynamicTableSource(DynamicTableFactory.Context c
boolean appendOnly =
config.get(SCAN_READ_CHANGELOG_AS_APPEND_ONLY_ENABLED);
boolean releaseSnapshotMetadataEnabled =
config.get(SCAN_INCREMENTAL_SNAPSHOT_METADATA_RELEASE_ENABLED);
+ double scanRateLimitRecordsPerSecond =
config.get(SCAN_RATE_LIMIT_RECORDS_PER_SECOND);
Review Comment:
Overall LGTM! I just have one suggestion for improvement: PostgreSQL SQL
sources default to `scan.incremental.snapshot.enabled=false`, which selects the
legacy `SourceFunction` path. In that case, a positive
`scan.rate-limit.records-per-second` value is accepted but silently ignored.
Could we add validation to reject this combination with a clear message
asking users to enable incremental snapshots, and document this requirement? A
regression test with a non-default rate limit would also help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
