DanielLeens commented on PR #11479: URL: https://github.com/apache/seatunnel/pull/11479#issuecomment-5178099349
Following up after @SEZ9's `LGTM, can merge` — and correcting my own previous comment, which was misleading. In my note of July 30 I wrote that the CI point was "in addition to the earlier source-level review items; those code-side blockers still need to be fixed as well." **That sentence was stale and I should not have written it.** My actual last review, on July 25, had already closed those items: > The earlier test-coverage blocker is closed for me now: the new composite common-handle cases exercise mixed key types through snapshot decoding and streaming PUT/DELETE decoding, rather than testing only the helper in isolation. I do not see a remaining source-level blocker in the current diff. So there is no disagreement here between @SEZ9 and me — we reached the same conclusion, and my July 30 comment simply contradicted my own earlier review. Sorry for the confusion that will have caused; if you were holding off waiting to hear which code-side items were still open, the answer is none. To be unambiguous about my current position on head `e0b5bc99`: **no source-level blocker from my side.** The four files in this diff (`CommonHandleDecoder`, both snapshot and streaming record deserializers, and the decoder test) still look correct to me, and the composite common-handle coverage exercises the decode path end to end rather than the helper in isolation. The remaining gate is CI, and the practical problem is that the signal attached to this head is not usable: the branch is `diverged` with `behind_by` in the dozens and the visible `Build` result predates a lot of `dev` movement. The failing lanes do not line up with a TiDB common-handle decoder change. So the lowest-cost next step is still to sync with the latest `dev` and rerun the check — not because syncing is a routine requirement, but specifically because the current red signal cannot be attributed to this diff and a refreshed one can. If it still fails after that, paste the fresh failing job link here and I will help narrow it down. After that, this needs a committer's approval to merge. -- 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]
