ricky2129 commented on PR #12555: URL: https://github.com/apache/seatunnel/pull/12555#issuecomment-5908679909
Flagging a problem with this before anyone spends review time on it. I raised the same change against Debezium (debezium/debezium#8135) and a maintainer there pointed out a data-loss path I had missed. In GTID mode the position comes from `client.setGtidSet(...)`, and the binlog filename is used only when there is no GTID set at all. That is what makes GTID mode safe when the source moves. This change reads `Previous_gtids` for the filename recorded in the checkpoint, but on whichever server the job happens to be connected to. If the source has been repointed to a promoted server, that server can hold a file of the same name whose `Previous_gtids` contains its own lineage. That lineage is not in the checkpoint, so `untrackedGtids` keeps it and those transactions are marked consumed without ever being read. As written it trades re-delivery for silent loss in the failover case, which is the worse of the two. Please don't merge it in this shape. The bug in #12554 stands on its own. The merge does claim an untracked lineage only as far as `gtid_purged`, and that is still unfixed on `dev`. It is the remedy here that is wrong. Worth noting what does not rescue it: inferring same-server by checking whether the checkpoint tracks `@@global.server_uuid`. server_uuid appears in `gtid_executed` only for transactions the server originated, so on a read replica the server's own lineage is absent from the set entirely and the check fails closed on exactly the case it is meant to permit. What it needs is for the checkpoint to record the source server's identity explicitly, so the filename is used only when the source has not changed. Same uuid means the file and position are genuinely ours and `Previous_gtids` answers it precisely; a different uuid, or an older checkpoint with nothing recorded, means we cannot know and the merge should stay as it is today. That only helps checkpoints written after the change, which is the real cost, but it never trades re-delivery for loss. Separately, the size of the problem is already computable inside `filterGtidSet` with no extra query and no behaviour change: for a lineage the server has and the checkpoint does not, `gtid_executed` minus `gtid_purged` is the number of transactions about to be re-delivered. Ours was around fifteen million with nothing in the log to say so, and that was the worse half of the incident. I will rework this once the Debezium discussion settles on a shape, since both projects carry the same merge. Happy to close this and open a fresh one with the revised approach if that is cleaner for review. -- 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]
