Re: [PR] feature: add console transaction control [incubator-seata]

["funky-eyes (via GitHub)" <gi...@apache.org>]

funky-eyes commented on code in PR #7024:
URL: https://github.com/apache/incubator-seata/pull/7024#discussion_r1900502136


##########
server/src/main/java/org/apache/seata/server/coordinator/AbstractCore.java:
##########
@@ -261,4 +264,25 @@ public GlobalStatus getStatus(String xid) throws TransactionException {
     public void doGlobalReport(GlobalSession globalSession, String xid, GlobalStatus globalStatus) throws TransactionException {
 
     }
+
+    @Override
+    public Boolean doBranchDelete(GlobalSession globalSession, BranchSession branchSession) throws TransactionException {
+        return true;
+    }
+
+    @Override
+    public BranchDeleteResponse branchDelete(GlobalSession globalSession, BranchSession branchSession) throws TransactionException {

Review Comment:
   BranchDeleteRequest 为什么要下发这个请求到client？如果需要发送该请求需要判断client版本是否支持该请求，我建议改为对应行为的request，比如二阶段回滚或提交的请求到client
   Why is the BranchDeleteRequest sent to the client? If this request needs to be sent, it should be determined whether the client version supports it. I suggest changing it to a corresponding behavior request, such as a two-phase rollback or commit request, sent to the client.



##########
server/src/main/java/org/apache/seata/server/session/BranchSession.java:
##########
@@ -423,6 +423,7 @@ private int calBranchSessionSize(byte[] resourceIdBytes, byte[] lockKeyBytes, by
             + 4 // applicationDataBytes.length
             + 4 // xidBytes.size
             + 1 // statusCode
+            + 8 // gmtModified

Review Comment:
   这里加了gmtModified，但是反序列化的时候又没有使用？要确认下这块到底有没有用，如果有用要保证向下兼容，因为低版本的是没有这个字段的，在raft和file模式下会存在兼容问题
   The gmtModified field is added here, but it is not used during deserialization. We need to confirm whether this field is actually needed. If it is, we must ensure backward compatibility, as older versions do not have this field. There may be compatibility issues in Raft and file modes.



##########
server/src/main/java/org/apache/seata/server/session/GlobalSession.java:
##########
@@ -724,6 +729,7 @@ public void decode(byte[] a) {
 
         this.beginTime = byteBuffer.getLong();
         this.status = GlobalStatus.get(byteBuffer.get());
+        this.gmtModified = byteBuffer.getLong();

Review Comment:
   需要向下兼容，不一定存在该值。
   Backward compatibility is required, and the value does not necessarily exist.



##########
tcc/src/main/java/org/apache/seata/rm/tcc/RMHandlerTCC.java:
##########
@@ -16,23 +16,54 @@
  */
 package org.apache.seata.rm.tcc;
 
+import org.apache.seata.core.model.BranchStatus;
 import org.apache.seata.core.model.BranchType;
 import org.apache.seata.core.model.ResourceManager;
+import org.apache.seata.core.protocol.ResultCode;
+import org.apache.seata.core.protocol.transaction.BranchDeleteRequest;
+import org.apache.seata.core.protocol.transaction.BranchDeleteResponse;
 import org.apache.seata.core.protocol.transaction.UndoLogDeleteRequest;
+import org.apache.seata.integration.tx.api.fence.DefaultCommonFenceHandler;
 import org.apache.seata.rm.AbstractRMHandler;
 import org.apache.seata.rm.DefaultResourceManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * The type Rm handler tcc.
  *
  */
 public class RMHandlerTCC extends AbstractRMHandler {
+    private static final Logger LOGGER = LoggerFactory.getLogger(RMHandlerTCC.class);
 
     @Override
     public void handle(UndoLogDeleteRequest request) {
         //DO nothing
     }
 
+    @Override
+    public BranchDeleteResponse handle(BranchDeleteRequest request) {
+        if (LOGGER.isDebugEnabled()) {
+            LOGGER.debug("Start tcc delete branch fence, xid:{}, branchId:{}",
+                    request.getXid(), request.getBranchId());
+        }
+        BranchDeleteResponse branchDeleteResponse = new BranchDeleteResponse();
+        try {
+            boolean result = DefaultCommonFenceHandler.get().
+                    deleteFenceByXidAndBranchId(request.getXid(), request.getBranchId());

Review Comment:
   我认为tcc的删除不能简单删除，因为如tcc一阶段是预留资源，如果被删除了，理论上应该走回滚逻辑，而deletefence本身有一个定时任务会去删除，故不需要专门的deleterequest，只要下发回滚即可，如果回滚失败应该提示删除失败，在提供一个强制删除的接口，针对回滚失败的事务，强删就是只删除服务端的session数据
   I believe that the deletion in TCC (Try-Confirm-Cancel) should not be a simple removal. Since TCC’s first phase reserves resources, if it's deleted, it should theoretically trigger a rollback logic. The deletefence already has a scheduled task to handle the deletion, so there’s no need for a dedicated deleterequest. Instead, a rollback should be issued. If the rollback fails, the deletion failure should be reported. Additionally, a forced deletion interface should be provided for transactions where the rollback fails. In this case, the forced deletion would only remove the session data from the server.



##########
common/src/main/java/org/apache/seata/common/result/Result.java:
##########
@@ -26,6 +26,8 @@ public class Result<T>  implements Serializable {
 
     public static final String SUCCESS_CODE = "200";
     public static final String SUCCESS_MSG = "success";
+    public static final String FAIL_CODE = "400";

Review Comment:
   400异常一般是指client异常状态码，服务端状态码应该取5xx的状态码
   A 400 error generally refers to a client-side error status code, while server-side errors should use 5xx status codes.



##########
server/src/main/java/org/apache/seata/server/session/GlobalSession.java:
##########
@@ -787,11 +793,17 @@ public void asyncCommit() throws TransactionException {
     }
 
     public void queueToRetryCommit() throws TransactionException {
+        if (this.status == GlobalStatus.StopCommitOrCommitRetry || this.status == GlobalStatus.StopRollbackOrRollbackRetry) {
+            return;
+        }
         changeGlobalStatus(GlobalStatus.CommitRetrying);
     }
 
     public void queueToRetryRollback() throws TransactionException {
         GlobalStatus currentStatus = this.getStatus();
+        if (currentStatus == GlobalStatus.StopCommitOrCommitRetry || currentStatus == GlobalStatus.StopRollbackOrRollbackRetry) {

Review Comment:
   与上面相反
   Contrary to the above



##########
server/src/main/java/org/apache/seata/server/coordinator/DefaultCoordinator.java:
##########
@@ -694,6 +694,14 @@ public void setRemotingServer(RemotingServer remotingServer) {
         this.remotingServer = remotingServer;
     }
 
+    /**
+     * get default core
+     * @return
+     */
+    public DefaultCore getCore() {

Review Comment:
   为什么添加这个方法？
   Why add this method?



##########
server/src/main/java/org/apache/seata/server/coordinator/AbstractCore.java:
##########
@@ -261,4 +264,25 @@ public GlobalStatus getStatus(String xid) throws TransactionException {
     public void doGlobalReport(GlobalSession globalSession, String xid, GlobalStatus globalStatus) throws TransactionException {
 
     }
+
+    @Override
+    public Boolean doBranchDelete(GlobalSession globalSession, BranchSession branchSession) throws TransactionException {
+        return true;
+    }
+
+    @Override
+    public BranchDeleteResponse branchDelete(GlobalSession globalSession, BranchSession branchSession) throws TransactionException {
+        BranchDeleteRequest request = new BranchDeleteRequest();
+        try {
+            request.setBranchType(branchSession.getBranchType());
+            request.setResourceId(branchSession.getResourceId());
+            request.setXid(globalSession.getXid());
+            request.setBranchId(branchSession.getBranchId());
+            return (BranchDeleteResponse) remotingServer.sendSyncRequest(
+                    branchSession.getResourceId(), branchSession.getClientId(), request, branchSession.isAT());
+        } catch (TimeoutException e) {
+            throw new BranchTransactionException(FailedToSendBranchDeleteRequest, String.format("Send branch delete failed," +

Review Comment:
   不需要这个状态，如果增加了版本判断，只可能出现timeout，timeout就是timeout，已经等于是失败了，就不需要这个状态了
   This status is not needed. If version checking is added, the only possible outcome would be a timeout. A timeout is essentially a failure, so this status is unnecessary.



##########
server/src/main/java/org/apache/seata/server/coordinator/DefaultCore.java:
##########
@@ -362,7 +396,7 @@ public boolean doGlobalRollback(GlobalSession globalSession, boolean retrying) t
 
         // In db mode, lock and branch data residual problems may occur.
         // Therefore, execution needs to be delayed here and cannot be executed synchronously.
-        if (success) {
+        if (success && globalSession.getBranchSessions().isEmpty()) {

Review Comment:
   为什么要额外加一个branchsessions是否为空？二阶段下发只要是没有异常，正常回滚了，该branchsessions肯定是为空的
   Why add an extra check for whether branchsessions is empty? In a two-phase commit, as long as there are no exceptions and the rollback is performed normally, the branchsessions should definitely be empty.



##########
server/src/main/java/org/apache/seata/server/session/GlobalSession.java:
##########
@@ -787,11 +793,17 @@ public void asyncCommit() throws TransactionException {
     }
 
     public void queueToRetryCommit() throws TransactionException {
+        if (this.status == GlobalStatus.StopCommitOrCommitRetry || this.status == GlobalStatus.StopRollbackOrRollbackRetry) {

Review Comment:
   StopRollbackOrRollbackRetry这个状态应该不会走到这里
   StopRollbackOrRollbackRetry this state shouldn't be here



-- 
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: notifications-unsubscribe@seata.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org