-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
refactor : separate the different transaction pattern processing logic #2134
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2134 +/- ##
=============================================
+ Coverage 53.04% 53.05% +<.01%
Complexity 2518 2518
=============================================
Files 488 488
Lines 15341 15341
Branches 1749 1749
=============================================
+ Hits 8138 8139 +1
+ Misses 6426 6425 -1
Partials 777 777
|
Are all three models regression tested with unit tests? |
Yes, I can add more if it's not enough |
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.
I have cloned you branch and tested SagaCore. It works OK
ok,thanks |
server/src/main/java/io/seata/server/coordinator/DefaultCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/seata/server/transaction/saga/SagaCore.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/seata/server/coordinator/AbstractCore.java
Outdated
Show resolved
Hide resolved
… RefactorServer_transaction
please resolve conflicts |
* @return the core | ||
*/ | ||
public AbstractCore getCore(BranchType branchType) { | ||
AbstractCore rm = coreMap.get(branchType); |
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.
What does rm mean?
@ph3636 DefaultCoordinatorTest test failed. |
this.messageSender = messageSender; | ||
} | ||
|
||
public abstract BranchType getBranchType(); |
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.
getHandleBranchType
done |
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.
LGTM
Ⅰ. Describe what this PR did
Ⅱ. 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