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

optimize: reduce the number of lock conflict exception #1469

Merged
merged 7 commits into from
Sep 10, 2019

Conversation

ggndnn
Copy link
Contributor

@ggndnn ggndnn commented Aug 16, 2019

Ⅰ. 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

@codecov-io
Copy link

codecov-io commented Aug 16, 2019

Codecov Report

Merging #1469 into develop will increase coverage by 0.47%.
The diff coverage is 48.78%.

Impacted file tree graph

@@              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
Impacted Files Coverage Δ Complexity Δ
...ava/io/seata/core/constants/ConfigurationKeys.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...n/java/io/seata/rm/datasource/ConnectionProxy.java 22.5% <44.44%> (+13.98%) 10 <1> (+6) ⬆️
...ta/rm/datasource/exec/AbstractDMLBaseExecutor.java 71.87% <57.14%> (+62.5%) 6 <1> (+4) ⬆️
.../rm/datasource/exec/BaseTransactionalExecutor.java 18.26% <0%> (+0.86%) 7% <0%> (+1%) ⬆️
...o/seata/rm/datasource/AbstractConnectionProxy.java 13.09% <0%> (+1.19%) 6% <0%> (+1%) ⬆️
...a/rm/datasource/exec/LockWaitTimeoutException.java 33.33% <0%> (+33.33%) 1% <0%> (+1%) ⬆️
...java/io/seata/rm/datasource/ConnectionContext.java 46.66% <0%> (+33.33%) 11% <0%> (+8%) ⬆️
.../seata/rm/datasource/exec/LockRetryController.java 92.3% <0%> (+92.3%) 4% <0%> (+4%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d334f85...14c9f3a. Read the comment docs.

@slievrly
Copy link
Member

slievrly commented Aug 16, 2019

@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.

@ggndnn
Copy link
Contributor Author

ggndnn commented Aug 16, 2019

@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 quick fail to avoid waiting too much. But rollback is not what we preferred, we prefer commit, according to issue 1438, we got exception immediately, may be just a moment we can get a success transaction. I think this PR is more in line with the figure in wiki. Is that a formal design?

I agree that we should evaluate this. What can we do with this PR?

@slievrly
Copy link
Member

@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?

@ggndnn
Copy link
Contributor Author

ggndnn commented Aug 19, 2019

@slievrly I joined DingTalk group.

@slievrly
Copy link
Member

@ggndnn Do you want to add a switch here as we communicate?

@ggndnn
Copy link
Contributor Author

ggndnn commented Aug 28, 2019

@slievrly Ok, I'll add a switch.

@ggndnn
Copy link
Contributor Author

ggndnn commented Sep 2, 2019

@slievrly I added a switch. Please have a look whether it's appropriate.

Copy link
Member

@slievrly slievrly left a 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();
Copy link
Member

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

Copy link
Contributor Author

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?

@xingfudeshi xingfudeshi self-requested a review September 7, 2019 12:24
Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

None yet

6 participants