DadaVinqi commented on code in PR #1148:
URL: 
https://github.com/apache/incubator-seata-go/pull/1148#discussion_r3877661512


##########
pkg/datasource/sql/xa_resource_manager.go:
##########
@@ -91,15 +92,14 @@ func (xaManager *XAResourceManager) 
xaTwoPhaseTimeoutChecker() {
                                if !ok {
                                        return true
                                }
-                               if source.IsShouldBeHeld() {
-                                       return true
-                               }
-
                                source.GetKeeper().Range(func(key, value any) 
bool {
                                        connectionXA, isConnectionXA := 
value.(*XAConn)
                                        if !isConnectionXA {
                                                return true
                                        }
+                                       if connectionXA.ShouldBeHeld() {
+                                               return true
+                                       }
 
                                        if 
time.Now().Sub(connectionXA.prepareTime) > xaManager.config.TwoPhaseHoldTime {

Review Comment:
   Thanks for catching this. The previous change conflated two different 
concepts: temporarily caching a connection in the keeper and requiring the 
original connection until phase two.
   
   I updated the implementation to separate them:
   
   - `keepInKeeper` controls whether the connection is temporarily cached.
   - `shouldBeHeld` controls whether timeout-based detach is allowed.
   - `preparedForPhaseTwo` explicitly records that XA PREPARE has completed.
   - `preparedForPhaseTwo`, `prepareTime`, and the hold policy are snapshotted 
under `keeperMu`.
   - The timeout sweep now only closes entries that are prepared, detachable, 
and older than `TwoPhaseHoldTime`.
   - Connections that must retain their original owner are still skipped 
intentionally.
   
   This makes the timeout sweep effective for detachable branches without 
closing owner-bound branches that cannot complete phase two on another 
connection.
   
   I also added regression coverage for unprepared entries, mandatory owner 
retention, recent detachable branches, and expired detachable branches.
   
   Addressed in a0c4b23.



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to