davidzollo commented on PR #10428:
URL: https://github.com/apache/seatunnel/pull/10428#issuecomment-3828341260

   
   Hi @LeonYoah, thanks for this contribution! The ability to start from a 
specific timestamp is a very valuable feature for CDC tasks.
   
   After reviewing the changes, I have a few concerns that need to be 
addressed, particularly regarding timezone handling in Oracle:
   
   ## 1. Critical Timezone Issue in `OracleConnectionUtils`
   
   In 
`seatunnel-connectors-v2/connector-cdc/connector-cdc-oracle/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/oracle/utils/OracleConnectionUtils.java`:
   
   You are currently converting the timestamp to a string using 
`timestamp.toString()` and then parsing it in Oracle:
   
   ```java
   String sql = "SELECT TIMESTAMP_TO_SCN(TO_TIMESTAMP(?, 'YYYY-MM-DD 
HH24:MI:SS.FF3')) AS SCN FROM DUAL";
   // ...
   statement.setString(1, timestamp.toString());
   ```
   
   **Risk:** `timestamp.toString()` relies on the JVM's default timezone to 
format the string. When `TO_TIMESTAMP` is executed in Oracle, it interprets the 
string based on the database session's timezone.
   If the SeaTunnel JVM timezone (e.g., UTC) is different from the Oracle 
Database timezone (e.g., Asia/Shanghai), the calculated SCN will be incorrect 
(potentially skewed by several hours), leading to data loss or duplication on 
startup.
   
   **Suggestion:** Use JDBC's `setTimestamp` directly, which is safer and 
handles the type conversion correctly:
   
   ```java
   String sql = "SELECT TIMESTAMP_TO_SCN(?) AS SCN FROM DUAL";
   // ...
   statement.setTimestamp(1, new java.sql.Timestamp(timestampMs));
   ```
   
   ## 2. Missing Documentation
   
   In `docs/en/connectors/source/Oracle-CDC.md` (and the cn version), you 
updated the description for `startup.mode` to include `timestamp`, which is 
great. However, the `startup.timestamp` configuration key itself is missing 
from the **Options** table.
   
   Users will not know that they need to configure `startup.timestamp` or that 
it requires a `Long` value (milliseconds). Please add this missing 
configuration item to the documentation.


-- 
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]

Reply via email to