-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
optimize: reduce the number of lock conflict exception #1469
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1469 +/- ##
=============================================
+ Coverage 46.78% 47.25% +0.47%
- Complexity 1730 1757 +27
=============================================
Files 353 353
Lines 12941 12967 +26
Branches 1618 1620 +2
=============================================
+ Hits 6054 6128 +74
+ Misses 6236 6178 -58
- Partials 651 661 +10
Continue to review full report at Codecov.
|
@ggndnn I am very interested because we always have different ideas. When a LockConflictException occurs, it indicates that there are other distributed transactions that are executing that hold the same data primary key. We define the current distributed transaction as A and another distributed transaction as B. A holds the database lock and wants to get the global lock, and B holds the global lock. If at this point B wants to rollback this data in the second phase of the distributed transaction, it will try to acquire the database lock. According to your code, A will hold the database lock for a longer time. At this time, B may trigger the lock wait timeout exception and perform a rollback retry. We need to evaluate this. |
@slievrly It's indeed interesting to have discussions with you, the atmosphere here is good, different ideas always make things more clear. Thank you. Before I make this PR, I referred to seata document in wiki, e.g. I think tx2 is A which you just mentioned and tx1 is B. I understand your worries. I also agree that we should make I agree that we should evaluate this. What can we do with this PR? |
@ggndnn I am very happy to discuss this with you. I understand what you mean above. By the way, is it appropriate for us to communicate through DingTalk? |
@slievrly I joined DingTalk group. |
@ggndnn Do you want to add a switch here as we communicate? |
@slievrly Ok, I'll add a switch. |
1489b29
to
b78b0a3
Compare
… lock when autocommit is false.
b78b0a3
to
a01ce0e
Compare
@slievrly I added a switch. Please have a look whether it's appropriate. |
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, but the code is not so easy to understand.
} catch (Exception e) { | ||
// when exception occur in finally,this exception will lost, so just print it here | ||
LOGGER.error("execute executeAutoCommitTrue error:{}", e.getMessage(), e); | ||
connectionProxy.rollback(); |
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.
just used for LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT=false
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.
Should it be okay to do ConnectionProxy#rollback
in both cases?
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
Move lock retry loop into ConnectionProxy.commit, that the number of LockKeyConflict can be significantly reduced whether auto-commit is true or false. Related issue #1438.
Ⅱ. 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