Skip to content
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

feature: add console transaction control #7024

Open
wants to merge 41 commits into
base: 2.x
Choose a base branch
from

Conversation

xjlgod
Copy link
Contributor

@xjlgod xjlgod commented Nov 22, 2024

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

feature: add console transaction control
the origin branch is summer-code_trxOPS
the doc can be seen in https://www.yuque.com/cairusigoudenanpeijiao/izo0ob/nyd5930611b4uag0?singleDoc#

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Copy link

codecov bot commented Dec 1, 2024

Codecov Report

Attention: Patch coverage is 24.88038% with 314 lines in your changes missing coverage. Please review.

Project coverage is 52.43%. Comparing base (2438d77) to head (ec3a0cf).

Files with missing lines Patch % Lines
...ata/server/console/impl/AbstractGlobalService.java 28.57% 59 Missing and 1 partial ⚠️
...ata/server/console/impl/AbstractBranchService.java 38.70% 31 Missing and 7 partials ⚠️
...che/seata/server/console/impl/AbstractService.java 56.33% 27 Missing and 4 partials ⚠️
...g/apache/seata/server/coordinator/DefaultCore.java 0.00% 29 Missing and 2 partials ⚠️
...erver/console/impl/db/GlobalLockDBServiceImpl.java 0.00% 19 Missing ⚠️
...er/console/controller/GlobalSessionController.java 5.26% 18 Missing ⚠️
...r/console/impl/file/GlobalLockFileServiceImpl.java 10.52% 17 Missing ⚠️
...console/impl/redis/GlobalLockRedisServiceImpl.java 0.00% 17 Missing ⚠️
...rver/console/aop/GlobalExceptionHandlerAdvice.java 14.28% 12 Missing ⚠️
...er/console/controller/BranchSessionController.java 7.69% 12 Missing ⚠️
... and 16 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #7024      +/-   ##
============================================
- Coverage     52.67%   52.43%   -0.24%     
- Complexity     6697     6745      +48     
============================================
  Files          1134     1140       +6     
  Lines         40310    40719     +409     
  Branches       4722     4791      +69     
============================================
+ Hits          21234    21353     +119     
- Misses        17043    17321     +278     
- Partials       2033     2045      +12     
Files with missing lines Coverage Δ
...in/java/org/apache/seata/common/result/Result.java 0.00% <ø> (ø)
...java/org/apache/seata/core/model/BranchStatus.java 100.00% <100.00%> (ø)
...java/org/apache/seata/core/model/GlobalStatus.java 97.67% <100.00%> (-2.33%) ⬇️
...er/console/impl/db/BranchSessionDBServiceImpl.java 0.00% <ø> (ø)
...er/console/impl/db/GlobalSessionDBServiceImpl.java 0.00% <ø> (ø)
...onsole/impl/file/GlobalSessionFileServiceImpl.java 82.35% <100.00%> (ø)
...org/apache/seata/server/session/SessionHolder.java 56.05% <ø> (+0.28%) ⬆️
...ver/storage/db/session/DataBaseSessionManager.java 59.45% <ø> (ø)
...ver/storage/redis/session/RedisSessionManager.java 62.85% <ø> (ø)
...sole/impl/redis/BranchSessionRedisServiceImpl.java 0.00% <0.00%> (ø)
... and 25 more

... and 10 files with indirect coverage changes

@xjlgod xjlgod changed the title [WIP] feature: add console transaction control feature: add console transaction control Dec 11, 2024
@jsbxyyx jsbxyyx added this to the 2.4.0 milestone Dec 31, 2024
@@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it.

}

@Override
public BranchDeleteResponse branchDelete(GlobalSession globalSession, BranchSession branchSession) throws TransactionException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have delete BranchDeleteRequest realted code.

return (BranchDeleteResponse) remotingServer.sendSyncRequest(
branchSession.getResourceId(), branchSession.getClientId(), request, branchSession.isAT());
} catch (TimeoutException e) {
throw new BranchTransactionException(FailedToSendBranchDeleteRequest, String.format("Send branch delete failed," +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不需要这个状态,如果增加了版本判断,只可能出现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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it.

* get default core
* @return
*/
public DefaultCore getCore() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么添加这个方法?
Why add this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it.

@@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么要额外加一个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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it.

@@ -423,6 +423,7 @@ private int calBranchSessionSize(byte[] resourceIdBytes, byte[] lockKeyBytes, by
+ 4 // applicationDataBytes.length
+ 4 // xidBytes.size
+ 1 // statusCode
+ 8 // gmtModified
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里加了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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deleted it.

@@ -724,6 +729,7 @@ public void decode(byte[] a) {

this.beginTime = byteBuffer.getLong();
this.status = GlobalStatus.get(byteBuffer.get());
this.gmtModified = byteBuffer.getLong();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deleted it.

@@ -787,11 +793,17 @@ public void asyncCommit() throws TransactionException {
}

public void queueToRetryCommit() throws TransactionException {
if (this.status == GlobalStatus.StopCommitOrCommitRetry || this.status == GlobalStatus.StopRollbackOrRollbackRetry) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deleted it.

changeGlobalStatus(GlobalStatus.CommitRetrying);
}

public void queueToRetryRollback() throws TransactionException {
GlobalStatus currentStatus = this.getStatus();
if (currentStatus == GlobalStatus.StopCommitOrCommitRetry || currentStatus == GlobalStatus.StopRollbackOrRollbackRetry) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

与上面相反
Contrary to the above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deleted it.

BranchDeleteResponse branchDeleteResponse = new BranchDeleteResponse();
try {
boolean result = DefaultCommonFenceHandler.get().
deleteFenceByXidAndBranchId(request.getXid(), request.getBranchId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我认为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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已按照意见进行更改,并新添加了全局和分支事务的强制session删除功能
Changes have been made in accordance with the comments, and new features have been added to force session deletion for global and branch transaction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants