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

bugfix: fix the case that could not retry acquire global lock #3133

Merged
merged 18 commits into from
Apr 16, 2021

Conversation

caohdgege
Copy link
Contributor

Ⅰ. Describe what this PR did

fix: will not retry acquire lock when LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT is true and autoCommit is false

Ⅱ. Does this pull request fix one issue?

fixes #3070

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2020

Codecov Report

Merging #3133 into develop will increase coverage by 0.00%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #3133   +/-   ##
==========================================
  Coverage      50.46%   50.47%           
- Complexity      3111     3112    +1     
==========================================
  Files            593      593           
  Lines          19571    19572    +1     
  Branches        2427     2427           
==========================================
+ Hits            9876     9878    +2     
+ Misses          8702     8701    -1     
  Partials         993      993           
Impacted Files Coverage Δ Complexity Δ
...n/java/io/seata/rm/datasource/ConnectionProxy.java 29.13% <66.66%> (+1.13%) 13.00 <1.00> (ø)
...ta/rm/datasource/exec/AbstractDMLBaseExecutor.java 75.00% <100.00%> (-0.61%) 9.00 <0.00> (ø)
...o/seata/rm/datasource/AbstractConnectionProxy.java 12.37% <0.00%> (+1.03%) 6.00% <0.00%> (+1.00%)

@caohdgege
Copy link
Contributor Author

image
补个图

@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #3133 (09c8d42) into develop (d8c09ab) will increase coverage by 0.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3133      +/-   ##
=============================================
+ Coverage      52.15%   52.16%   +0.01%     
- Complexity      3509     3510       +1     
=============================================
  Files            638      638              
  Lines          21109    21110       +1     
  Branches        2614     2615       +1     
=============================================
+ Hits           11009    11012       +3     
+ Misses          9014     9012       -2     
  Partials        1086     1086              
Impacted Files Coverage Δ Complexity Δ
...n/java/io/seata/rm/datasource/ConnectionProxy.java 25.69% <66.66%> (+1.04%) 14.00 <2.00> (+1.00)
...ta/rm/datasource/exec/AbstractDMLBaseExecutor.java 58.82% <100.00%> (-0.80%) 11.00 <0.00> (ø)
...java/io/seata/rm/datasource/ConnectionContext.java 82.47% <0.00%> (+1.03%) 37.00% <0.00%> (+1.00%)
...o/seata/rm/datasource/AbstractConnectionProxy.java 13.18% <0.00%> (+1.09%) 6.00% <0.00%> (+1.00%)

@ggndnn
Copy link
Contributor

ggndnn commented Oct 16, 2020

@caohdgege I think LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT==true means to release local record lock before retry when global lock conflict, that was not achieved by this pr.

@caohdgege
Copy link
Contributor Author

caohdgege commented Oct 16, 2020

@caohdgege I think LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT==true means to release local record lock before retry when global lock conflict, that was not achieved by this pr.

这个之前在群里跟清铭哥讨论过,在autocommit=false 的时候,进行释放本地锁有点难,然后重试策略又比较重要,暂时先通过让它不释放本地锁直接重试,后续再优化

@ggndnn
Copy link
Contributor

ggndnn commented Oct 17, 2020

@caohdgege I think LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT==true means to release local record lock before retry when global lock conflict, that was not achieved by this pr.

这个之前在群里跟清铭哥讨论过,在autocommit=false 的时候,进行释放本地锁有点难,然后重试策略又比较重要,暂时先通过让它不释放本地锁直接重试,后续再优化

If we have to do this, should we change the configuration meaning? Otherwise, it is easy to cause misunderstanding. By the way, how about to make use of savepoint?

@caohdgege
Copy link
Contributor Author

caohdgege commented Oct 17, 2020

@caohdgege I think LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT==true means to release local record lock before retry when global lock conflict, that was not achieved by this pr.

这个之前在群里跟清铭哥讨论过,在autocommit=false 的时候,进行释放本地锁有点难,然后重试策略又比较重要,暂时先通过让它不释放本地锁直接重试,后续再优化

If we have to do this, should we change the configuration meaning? Otherwise, it is easy to cause misunderstanding. By the way, how about to make use of savepoint?

我个人的理解上,如果在 autocommit=false 的时候,如果要释放本地锁然后重试意味着要 重新执行对应的DML以及业务操作 ,用savepoint或者记录到context只能保证重新执行对应的DML,没有执行对应的业务操作。现在没能找到一个方案可以实现重新执行对应的DML以及业务操作,所以才采用了这么一个临时方案。或者云嵩哥有没有什么更好的建议?
对于修改配置的定义,云嵩哥觉得改成什么好?

@ggndnn
Copy link
Contributor

ggndnn commented Oct 17, 2020

@caohdgege I think LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT==true means to release local record lock before retry when global lock conflict, that was not achieved by this pr.

这个之前在群里跟清铭哥讨论过,在autocommit=false 的时候,进行释放本地锁有点难,然后重试策略又比较重要,暂时先通过让它不释放本地锁直接重试,后续再优化

If we have to do this, should we change the configuration meaning? Otherwise, it is easy to cause misunderstanding. By the way, how about to make use of savepoint?

我个人的理解上,如果在 autocommit=false 的时候,如果要释放本地锁然后重试意味着要 重新执行对应的DML以及业务操作 ,用savepoint或者记录到context只能保证重新执行对应的DML,没有执行对应的业务操作。现在没能找到一个方案可以实现重新执行对应的DML以及业务操作,所以才采用了这么一个临时方案。或者云嵩哥有没有什么更好的建议?
对于修改配置的定义,云嵩哥觉得改成什么好?

I think biz op is something bigger than things here, the global lock keys are also from dml and db values, we can consider this configuration only in at mode so far. Under this consideration, what about savepoint?
As for the configuration naming, let's think about it together, maybe more discussions. When the configuration changed, the code logic may have to be changed.

@wangliang181230 wangliang181230 added this to the 1.5.0 milestone Oct 29, 2020
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, the retry functionality is fine, but the meaning of the configuration item has changed and may require further consideration.

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

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

LGTM

@caohdgege caohdgege changed the title bugfix: fix could not retry acquire global lock when LOCK_RETRY_POLICY_BRANCH_ROLLBACK_ON_CONFLICT is true and autoCommit is false bugfix: fix the case that could not retry acquire global lock Apr 16, 2021
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.

Could not retry get global lock when autocommit=false
7 participants