JeremyXin commented on PR #11569: URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5371684900
Reviewed the latest head 61df8fd43. The changes look good overall — found two issues worth confirming before merge: **Issue 1**: No backoff between synchronous XA commit/recover retry rounds (MAJOR) Location: JdbcSinkAggregatedCommitter.java — commitXidInfos (L129-142) / recoverCheckpointTransactions (L237-248) Description: Both bounded retry loops are tight synchronous loops with no delay between rounds. For XAER_RMFAIL-class outages (resource manager unavailable), the whole max_commit_attempts budget is consumed in microseconds, making the retry mechanism effectively inert for the exact failure class it was meant to absorb. The same applies to the recovery-scan retry in restoreCommit. Suggestion: Introduce a bounded backoff (e.g. a fixed 1s delay, or capped exponential backoff) between synchronous retry rounds in both commitXidInfos and recoverCheckpointTransactions, so transient RM unavailability is actually absorbed by the retry budget instead of being burned instantly. **Issue 2**: No integration/real-database test exercising XA driver-level semantics (MAJOR) Location: XaGroupOpsImplIT.java (connector-jdbc-e2e-part-1); the three new test classes (XaFacadeImplAutoLoadTest, XaGroupOpsImplTest, JdbcSinkAggregatedCommitterTest) are Mockito-based unit tests only Description: The core of this PR is XA error-code classification (XA_RETRY/XAER_RMFAIL as transient vs XA_RBTRANSIENT as permanent) and the restore/recovery behavior, both of which depend on what the real database driver actually returns (e.g. how MySQL/PostgreSQL surface XAER_NOTA, XA_RETRY, XAER_RMFAIL, and how XAResource.recover() returns driver-specific Xid values). Mock-based unit tests can verify that the classification logic is internally consistent, but they cannot verify that the real drivers behave as assumed. This leaves a gap between the tested behavior and the actual runtime contract. Suggestion: Re-enable XaGroupOpsImplIT (it remains @Disabled) with a real database, covering at least one end-to-end path where an XA commit/restore failure propagates through to a checkpoint/job failure — this directly validates the failure-propagation behavior this PR is fixing. -- 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]
