DanielLeens commented on PR #11408: URL: https://github.com/apache/seatunnel/pull/11408#issuecomment-5203554803
Thanks @SEZ9 for the very thorough re-check. To confirm the sequencing: this is still the same head (`f0b0eb1e1d36`, the only commit on the branch) that I reviewed on July 11 and rechecked on July 14 — no new commit has landed since, so I'm reconciling with your review rather than doing a fresh pass. I independently re-verified each blocking item against the diff and agree with all four: +1 on Issue 2 (dead code): confirmed. In both `TiDBSourceReader.captureStreamingEvent` (`solution_0.py`) and `CDCClient.handleResolvedTs` (`solution_1.py`), `lastKnownGoodTs` is assigned `= self.resolvedTs`/`= resolvedTs` in the very same block that just advanced `resolvedTs`, so the difference is always `0` and the `> 1000` branch in `checkResolvedTs` can never execute. The "reset to last known good" behavior this PR advertises never actually runs. +1 on Issue 4 (None-default crash) with one additional piece of evidence: this PR actually ships two conflicting implementations of the same classes. `solution_0.py`/`solution_1.py` initialize `resolvedTs`/`lastKnownGoodTs` to a concrete `0` in `__init__`. But `solution_4.py`/`solution_5.py` redeclare `TiDBSourceReader`/`CDCClient` a second time and instead copy the values straight from `TiDBSourceReaderConfig`/`CDCClientConfig` (`solution_2.py`/`solution_3.py`), which default both fields to `None`. So beyond the `None`-arithmetic crash you flagged, it isn't even clear which of the two competing constructors is meant to be kept. Issue 3 lines up with what I raised as Issue 1 on July 11 and reiterated on July 14: none of `solution_0.py`–`solution_5.py` sit inside `seatunnel-connectors-v2/connector-tidb-cdc` or anywhere else in the Maven build, so no SeaTunnel job — Zeta, Flink, or Spark — executes any of this. That remains the fundamental blocker independent of the internal logic bugs you found. Keeping this marked as changes-requested from my side, unchanged from before. @TFGSUMIT, once there's a real Java-side commit against the TiDB-CDC connector, I'm glad to take a fresh full pass. -- 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]
