-
Notifications
You must be signed in to change notification settings - Fork 8.9k
Feature 7840 saga annotation robustness #8027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.x
Are you sure you want to change the base?
Changes from 2 commits
24301a7
c1ef0b1
26eff1e
2513f05
0b64f2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,13 +16,15 @@ | |||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| package org.apache.seata.integration.tx.api.interceptor; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.ConfigurationKeys; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.Constants; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.exception.FrameworkException; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.exception.SkipCallbackWrapperException; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.executor.Callback; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.json.JsonUtil; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.util.CollectionUtils; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.util.NetUtil; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.config.ConfigurationFactory; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.core.context.RootContext; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.integration.tx.api.fence.DefaultCommonFenceHandler; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.integration.tx.api.fence.hook.TccHook; | ||||||||||||||||||||||||||||||||||||
|
|
@@ -53,6 +55,9 @@ public class ActionInterceptorHandler { | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static final Logger LOGGER = LoggerFactory.getLogger(ActionInterceptorHandler.class); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static final boolean ENABLE_ACTION_STATUS_REPORT = ConfigurationFactory.getInstance() | ||||||||||||||||||||||||||||||||||||
| .getBoolean(ConfigurationKeys.CLIENT_SAGA_ACTION_STATUS_REPORT_ENABLE, false); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Handler the Tx Aspect | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
|
|
@@ -111,7 +116,20 @@ public Object proceed( | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||
| // Execute business, and return the business result | ||||||||||||||||||||||||||||||||||||
| return targetCallback.execute(); | ||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||
| Object result = targetCallback.execute(); | ||||||||||||||||||||||||||||||||||||
| // Report action status: success (only for non-CommonFence mode) | ||||||||||||||||||||||||||||||||||||
| if (ENABLE_ACTION_STATUS_REPORT) { | ||||||||||||||||||||||||||||||||||||
| reportActionStatus(actionContext, Constants.ACTION_STATUS_SUCCESS); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||
| } catch (Throwable t) { | ||||||||||||||||||||||||||||||||||||
| // Report action status: failed (only for non-CommonFence mode) | ||||||||||||||||||||||||||||||||||||
| if (ENABLE_ACTION_STATUS_REPORT) { | ||||||||||||||||||||||||||||||||||||
| reportActionStatus(actionContext, Constants.ACTION_STATUS_FAILED); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| throw t; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -325,4 +343,32 @@ protected Map<String, Object> fetchActionRequestContext(Method method, Object[] | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return context; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Report action status to TC | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * @param actionContext the action context | ||||||||||||||||||||||||||||||||||||
| * @param status the action status (success/failed) | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| protected void reportActionStatus(BusinessActionContext actionContext, String status) { | ||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||
| actionContext.setActionStatus(status); | ||||||||||||||||||||||||||||||||||||
| actionContext.setUpdated(true); | ||||||||||||||||||||||||||||||||||||
| BusinessActionContextUtil.reportContext(actionContext); | ||||||||||||||||||||||||||||||||||||
| if (LOGGER.isDebugEnabled()) { | ||||||||||||||||||||||||||||||||||||
| LOGGER.debug( | ||||||||||||||||||||||||||||||||||||
| "Report action status: xid={}, branchId={}, status={}", | ||||||||||||||||||||||||||||||||||||
| actionContext.getXid(), | ||||||||||||||||||||||||||||||||||||
| actionContext.getBranchId(), | ||||||||||||||||||||||||||||||||||||
| status); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||||||||||||||||||||
| LOGGER.warn( | ||||||||||||||||||||||||||||||||||||
| "Report action status failed: xid={}, branchId={}, status={}, error={}", | ||||||||||||||||||||||||||||||||||||
| actionContext.getXid(), | ||||||||||||||||||||||||||||||||||||
| actionContext.getBranchId(), | ||||||||||||||||||||||||||||||||||||
| status, | ||||||||||||||||||||||||||||||||||||
| e.getMessage()); | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+366
to
+371
|
||||||||||||||||||||||||||||||||||||
| LOGGER.warn( | |
| "Report action status failed: xid={}, branchId={}, status={}, error={}", | |
| actionContext.getXid(), | |
| actionContext.getBranchId(), | |
| status, | |
| e.getMessage()); | |
| // Make status reporting best-effort: roll back the updated flag on failure | |
| actionContext.setUpdated(false); | |
| LOGGER.warn( | |
| "Report action status failed: xid={}, branchId={}, status={}", | |
| actionContext.getXid(), | |
| actionContext.getBranchId(), | |
| status, | |
| e); |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning log drops the exception stack trace, which makes diagnosing production/reporting failures harder. Pass e as the last argument to the logger (and optionally remove error={}/e.getMessage()), so stack traces are captured in logs.
| e.getMessage()); | |
| e.getMessage(), | |
| e); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.seata.rm.tcc.api; | ||
|
|
||
| import org.apache.seata.common.Constants; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| /** | ||
| * Test cases for BusinessActionContext action status methods. | ||
| */ | ||
| public class BusinessActionContextTest { | ||
|
|
||
| @Test | ||
| public void testGetActionStatusWhenContextIsNull() { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
| assertNull(context.getActionStatus(), "Action status should be null when actionContext is null"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetActionStatusWhenNotSet() { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
| Map<String, Object> actionContext = new HashMap<>(); | ||
| context.setActionContext(actionContext); | ||
|
|
||
| assertNull(context.getActionStatus(), "Action status should be null when not set"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetAndGetActionStatusSuccess() { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
| Map<String, Object> actionContext = new HashMap<>(); | ||
| context.setActionContext(actionContext); | ||
|
|
||
| context.setActionStatus(Constants.ACTION_STATUS_SUCCESS); | ||
|
|
||
| assertEquals(Constants.ACTION_STATUS_SUCCESS, context.getActionStatus(), "Action status should be 'success'"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetAndGetActionStatusFailed() { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
| Map<String, Object> actionContext = new HashMap<>(); | ||
| context.setActionContext(actionContext); | ||
|
|
||
| context.setActionStatus(Constants.ACTION_STATUS_FAILED); | ||
|
|
||
| assertEquals(Constants.ACTION_STATUS_FAILED, context.getActionStatus(), "Action status should be 'failed'"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetActionStatusWithNullStatus() { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
| Map<String, Object> actionContext = new HashMap<>(); | ||
| context.setActionContext(actionContext); | ||
|
|
||
| context.setActionStatus(Constants.ACTION_STATUS_SUCCESS); | ||
| context.setActionStatus(null); | ||
|
|
||
| assertEquals( | ||
| Constants.ACTION_STATUS_SUCCESS, | ||
| context.getActionStatus(), | ||
| "Action status should not change when setting null"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetActionStatusWithNullActionContext() { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
|
|
||
| // Should not throw exception | ||
| assertDoesNotThrow( | ||
| () -> context.setActionStatus(Constants.ACTION_STATUS_SUCCESS), | ||
| "Setting action status with null actionContext should not throw exception"); | ||
|
|
||
| assertNull(context.getActionStatus(), "Action status should still be null"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testActionStatusOverwrite() { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
| Map<String, Object> actionContext = new HashMap<>(); | ||
| context.setActionContext(actionContext); | ||
|
|
||
| context.setActionStatus(Constants.ACTION_STATUS_SUCCESS); | ||
| assertEquals(Constants.ACTION_STATUS_SUCCESS, context.getActionStatus()); | ||
|
|
||
| context.setActionStatus(Constants.ACTION_STATUS_FAILED); | ||
| assertEquals( | ||
| Constants.ACTION_STATUS_FAILED, | ||
| context.getActionStatus(), | ||
| "Action status should be overwritten to 'failed'"); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,12 +23,15 @@ | |||||||||
| import org.apache.seata.core.model.BranchStatus; | ||||||||||
| import org.apache.seata.core.model.BranchType; | ||||||||||
| import org.apache.seata.core.model.Resource; | ||||||||||
| import org.apache.seata.integration.tx.api.fence.hook.TccHook; | ||||||||||
| import org.apache.seata.integration.tx.api.fence.hook.TccHookManager; | ||||||||||
| import org.apache.seata.integration.tx.api.remoting.TwoPhaseResult; | ||||||||||
| import org.apache.seata.rm.AbstractResourceManager; | ||||||||||
| import org.apache.seata.rm.tcc.api.BusinessActionContext; | ||||||||||
| import org.apache.seata.rm.tcc.api.BusinessActionContextUtil; | ||||||||||
|
|
||||||||||
| import java.lang.reflect.Method; | ||||||||||
| import java.util.List; | ||||||||||
| import java.util.Map; | ||||||||||
| import java.util.concurrent.ConcurrentHashMap; | ||||||||||
|
|
||||||||||
|
|
@@ -100,12 +103,15 @@ public BranchStatus branchRollback( | |||||||||
| String.format("SagaAnnotation resource is not available, resourceId: %s", resourceId)); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| BusinessActionContext businessActionContext = null; | ||||||||||
| try { | ||||||||||
| BusinessActionContext businessActionContext = | ||||||||||
| businessActionContext = | ||||||||||
| BusinessActionContextUtil.getBusinessActionContext(xid, branchId, resourceId, applicationData); | ||||||||||
| Object[] args = this.getTwoPhaseRollbackArgs(resource, businessActionContext); | ||||||||||
| BusinessActionContextUtil.setContext(businessActionContext); | ||||||||||
|
Comment on lines
+106
to
111
|
||||||||||
|
|
||||||||||
| doBeforeSagaAnnotationRollback(xid, branchId, resource.getActionName(), businessActionContext); | ||||||||||
|
|
||||||||||
| boolean result; | ||||||||||
| Object ret = compensationMethod.invoke(targetBean, args); | ||||||||||
| if (ret != null) { | ||||||||||
|
|
@@ -131,6 +137,7 @@ public BranchStatus branchRollback( | |||||||||
| LOGGER.error(msg, ExceptionUtil.unwrap(t)); | ||||||||||
| return BranchStatus.PhaseTwo_RollbackFailed_Retryable; | ||||||||||
| } finally { | ||||||||||
| doAfterSagaAnnotationRollback(xid, branchId, resource.getActionName(), businessActionContext); | ||||||||||
|
||||||||||
| doAfterSagaAnnotationRollback(xid, branchId, resource.getActionName(), businessActionContext); | |
| if (businessActionContext != null) { | |
| doAfterSagaAnnotationRollback(xid, branchId, resource.getActionName(), businessActionContext); | |
| } |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The log message "Failed execute beforeTccRollback in hook {}" is grammatically incorrect and the {} placeholder is currently used for the exception message, which loses useful context about which hook failed. Consider changing the message to “Failed to execute …” and log hook identity (e.g., hook class/name) in the placeholder; keep the exception as the throwable argument for stack trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENABLE_ACTION_STATUS_REPORTis read once at class-load time. This prevents config hot-reload from taking effect and can also capture the wrong value ifConfigurationFactoryisn’t initialized yet when this class is first loaded. Prefer reading the flag fromConfigurationFactory.getInstance()at execution time (or caching via a config listener) instead of astatic finalsnapshot.