DanielLeens commented on PR #11479: URL: https://github.com/apache/seatunnel/pull/11479#issuecomment-5633105357
@zhangshenghang Thanks for the resync. I checked the current PR state fresh: the required `Build` check is now `SUCCESS` (https://github.com/apache/seatunnel/runs/99833427268), so the resync worked. I also want to be precise about what "no code changes since the last push" covers. The head did move since my last comment — from `30c35a30a438` (2026-08-26) to `446cf0ef20b2` ("Resolve merge conflicts with latest dev", 2026-09-01) — but I checked `compare/30c35a30a438...446cf0ef20b2` and confirmed none of the TiDB CDC common-handle files are in that diff (no `CommonHandleDecoder`, `SeaTunnelRowSnapshotRecordDeserializer`, `SeaTunnelRowStreamingRecordDeserializer`, or `CommonHandleDecoderTest`). So you're right that it's a dev-sync merge commit with no source changes on this PR's own diff — I'm treating this as the same reviewed head for the purposes of F2-F8 below, not a new version needing a fresh full re-review. To be clear about where this PR actually stands, since CI passing on its own doesn't change the answer: the CI signal was never the blocking item here. @SEZ9 and I aligned back on 2026-08-26 that F1 was withdrawn (my correction on the PUT-branch read was confirmed), but F2-F8 are still open and unresolved on this unchanged code: - **F2** — whether `RowKey.decode(...).getHandle()` in the snapshot deserializer can throw on a short common-handle key before `restorePrimaryKeyColumns` runs; needs a trace through the tikv-client-java key-decoding path or a regression test covering a sub-8-byte encoded handle. - **F3** — the all-or-nothing restore in `CommonHandleDecoder` can overwrite PK values already present in the row value; should be a per-column null check instead. - **F4** — missing length/type guard before the fixed `skipBytes(11)` in `restorePrimaryKeyColumns`. - **F5** — missing class-level Javadoc on `CommonHandleDecoder` and no comment explaining `RECORD_KEY_PREFIX_LENGTH = 11`. - **F6** — missing test coverage for a streaming PUT with non-empty `oldValue`, and for a `pkHandle` / non-clustered-PK table where the guard must skip. - **F7** — `TiTableInfo.getIndices()` is not null-guarded in `primaryIndex()`. - **F8** (non-blocking) — duplicate `ByteString.toByteArray()` copies and a per-record linear primary-index scan in the streaming deserializer. F2 and F4 are the two I'd treat as most load-bearing, since both sit directly on the decode path rather than being polish items. None of these are CI-shaped — they need an actual code change. Once a new commit addresses F2-F7 (plus the F5 Javadoc), ping me and @SEZ9 and we'll do the joint verification pass we agreed on. -- 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]
