nzw921rx commented on PR #12807: URL: https://github.com/apache/seatunnel/pull/12807#issuecomment-6459936494
Thanks for the focused fix. I checked the restore call chain and ran local regression tests on PR head `c3437e23a79733fe996fdcb14d647ce596488178`, with `dev` at `403efeee0b3806c77e55c38368abdd41fbf828bd` merged locally. The forwarding change is correct: normal commits still call each table's `commit()`; restores now call each table's `restoreCommit()`. Committers without an override continue to use the interface default, which delegates to `commit()`. ### Unit tests and build checks | Scope | Result | |---|---| | API unit tests | 410 passed | | JDBC XA unit tests | 28 passed | | Iceberg unit tests | 131 passed | | MySQL CDC configuration/serialization tests | 11 passed | | Supplemental multi-table wrapper boundary tests | 8 passed | | Runtime dependency build | 47-module build passed | | API formatting and verification | Spotless check and Maven verify passed; tests were run separately as listed above | All 588 tests listed above completed without failures, errors, or skips. The eight supplemental wrapper tests covered: - Per-table routing, input ordering, and missing/null table entries. - Unequal, non-empty retry lists, preserving the correct table identifiers. - The default `restoreCommit()` implementation delegating to `commit()`. - Normal `commit()` not invoking a custom restore override. - Exceptions remaining visible, with the existing fail-fast behavior. - Empty restored state. - Repeated restore calls preserving each table's batch sequence. - Serialization round-trips preserving exact table identifiers. These supplemental tests were run locally; they have not been added to this PR. ### Real MySQL recovery tests The tests used MySQL 8.0.43 and actual MySQL-CDC → JDBC jobs. Two Master instances and one Worker instance were started in one JVM and communicated over loopback networking. This validates actual connector and engine execution, but is not a multi-process or multi-host deployment test. The successful job scenarios included initial snapshots, streaming inserts/updates/deletes, checkpoint cancellation and restore, and two further savepoint/restore cycles. Source and target rows were compared after recovery and subsequent DML. Transactional target audit triggers also checked that successful XA restores did not replay writes. For the negative controls, the dev wrapper implementation was substituted into the same runtime while keeping the remaining components unchanged. | Scenario | Dev control | PR result | |---|---|---| | Two-table XA state containing already-committed transactions | Reproduced `XAER_NOTA` | Repeated restore three times successfully | | XA recovery matrix: normal commits, all-prepared transactions, committed-prefix/prepared-suffix recovery, unrelated prepared transactions, inconsistent recovery gaps, and empty state | Confirmed the original restore failure separately | 13 checks passed, including the expected rejection of an inconsistent gap | | Single-table JDBC XA | Restore failed with `XAER_NOTA` | Three restore cycles and subsequent DML passed; final 32 rows matched | | Three-table JDBC without XA, source `exactly_once=true` | Three restore cycles passed | Three restore cycles passed; final 96 rows matched | | Three-table JDBC without XA, source `exactly_once=false` | Three restore cycles passed | Three restore cycles passed; final 96 rows matched | | Three-table JDBC XA, parallelism 2 and `multi_table_sink_replica=2` | Original restore failure reproduced | The old checkpoint transactions were handled correctly, but a subsequent normal commit failed due to the separate cleanup issue below; reproduced again in a fresh run | | Three-table JDBC XA, parallelism 1 and replica 1 | — | The same subsequent-commit failure reproduced | | Three-table JDBC XA with source `exactly_once=false` | Original restore failure reproduced | The same subsequent-commit failure reproduced | For JDBC with `is_exactly_once=false`, no JDBC aggregated committer is created, so this change does not alter that JDBC recovery path. Turning off source exactly-once alone does not remove the sink's XA recovery requirements. ### Real Iceberg recovery tests I also exercised the production `IcebergAggregatedCommitter` through the multi-table wrapper using two HadoopCatalog tables and actual Parquet files. | Scenario | Result | |---|---| | Restore an already-committed checkpoint | Dev appended the same files again: each table grew from 2 to 4 rows. With the PR, three repeated restores preserved row counts and snapshot IDs | | Restore pending files | Files committed once | | One table already committed, another still pending | Both tables recovered without duplicate appends | | Checkpoint ID zero / existing-file fallback | Passed | | Empty restore state | Passed | | Serialize commit state, create fresh committers, and restore against the existing catalog | Passed; snapshots unchanged | Nine Iceberg checks passed. This was a separate local connector recovery test, rather than a run of the existing Iceberg savepoint IT. ### Separate, pre-existing JDBC XA cleanup issue The multi-table XA job failures above are important to distinguish from this PR's forwarding fix. After restoration, a table writer can prepare a valid new transaction before another table writer first opens. The latter calls `recoverAndRollback()`. Its ownership check matches the job ID and subtask index, without distinguishing the other table's branch, so it can roll back that valid peer transaction. The next normal checkpoint commit then fails with `XAER_NOTA`. I reproduced this independently with real prepared MySQL XA branches on both dev and the PR: - Two table branches were prepared. - Production recovery cleanup reduced the prepared count from two to one. - Committing the other table's branch returned `XAER_NOTA`. In the complete PR job, the logs first showed the restored checkpoint's already-resolved XIDs being skipped correctly. The later failure involved a new transaction and occurred through normal `notifyCheckpointComplete → commit()`, rather than the restored checkpoint's commit path. As a diagnostic control only, narrowing cleanup ownership in an isolated local copy preserved both prepared branches and allowed the same three-table job to complete all three restores and subsequent DML, ending with 96 matching rows and 204 audit records. This experimental change is not part of the PR, is not presented as a complete production fix, and its successful run is not counted as a successful run of the unmodified PR. This existing JDBC issue should be tracked separately. It does not invalidate the wrapper forwarding fix, but the results should not be described as closing every multi-table XA recovery scenario. An additional abrupt-Master-termination probe in the embedded setup timed out and remains inconclusive; it is not counted among the passing tests or attributed to this PR. I support the focused forwarding change. The local tests confirm the intended dispatch, unchanged default behavior, and the JDBC/Iceberg recovery improvement. -- 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]
