DanielCarter-stack commented on PR #10604:
URL: https://github.com/apache/seatunnel/pull/10604#issuecomment-4068157982
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10604", "part": 1,
"total": 1} -->
#### Issue 1: Typo
**Location**:
-
`seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/option/JdbcSourceOptions.java:153`
-
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/config/JdbcSourceOptions.java:106`
**Issue Description**:
There is a typo in the configuration description: "The default value is
ture." should be "The default value is true."
**Potential Risks**:
- Users will see the typo when reading the documentation, affecting
professionalism
- May lead users to question code quality
**Impact Scope**:
- All users using CDC and JDBC connectors
**Severity**: MINOR
**Improvement Suggestion**:
```java
.withDescription(
"Whether to enable sampling-based sharding strategy."
+ "When set false, the system should fall back to unevenly-sized
chunk splitting (iterative query approach) regardless of the shard count."
+ "The default value is true."); // Fix typo
```
#### Issue 2: Missing E2E tests for CDC
**Location**:
- All CDC connectors (MySQL, Oracle, PostgreSQL, SQLServer)
**Issue Description**:
The PR only added E2E tests for JDBC connectors, but did not add
corresponding E2E tests for CDC connectors. Although the code logic is the same
as JDBC, CDC has different execution paths, and independent E2E tests would
provide better assurance.
**Potential Risks**:
- Changes to CDC connectors may have undiscovered integration issues
- If CDC and JDBC logic diverge in the future, it may not be detected in time
**Impact Scope**:
- All CDC connector users
**Severity**: MINOR
**Improvement Suggestion**:
It is recommended to add similar E2E tests for CDC connectors, for example
by adding test methods in each CDC's E2E test class. However, this is not
required because:
1. Unit tests have already covered the core logic
2. JDBC E2E tests have verified functional correctness
3. CDC and JDBC use the same underlying code
#### Issue 3: Missing unit tests for CDC connectors
**Location**:
- Configuration tests for all CDC connectors
**Issue Description**:
The PR only added unit tests for JDBC
connectors`testSampleShardingEnableConfigParsing`, and did not add similar
configuration parsing tests for CDC connectors.
**Potential Risks**:
- Configuration parsing for CDC connectors may have undiscovered bugs
- Constructor parameter passing for each CDC connector may have issues
**Impact Scope**:
- All CDC connectors
**Severity**: MINOR
**Improvement Suggestion**:
Although not required (since the configuration parsing logic is in the base
class), consider adding similar configuration parsing tests in each CDC
connector's test class to ensure completeness.
--
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]
