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

session: make tidb_disable_txn_auto_retry disable retry for the explicit transactions #10339

Merged
merged 2 commits into from May 5, 2019

Conversation

Projects
None yet
6 participants
@jackysp
Copy link
Member

commented May 4, 2019

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What problem does this PR solve?

It is not a good idea to make tidb_disable_txn_auto_retry deal with the write-conflict error only. Some rigorous tests e.g. jepsen will find that this will make the transaction behave abnormally. Although we can set tidb_retry_limit = 0 to disable retry either.

What is changed and how it works?

Make tidb_disable_txn_auto_retry work the same as before, it will disable retry for the explicit transactions, no matter what kind of error.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
make tidb_disable_txn_auto_retry as it used to be
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

@jackysp jackysp requested review from AndreMouche, disksing and lysu May 4, 2019

@codecov

This comment has been minimized.

Copy link

commented May 4, 2019

Codecov Report

Merging #10339 into master will decrease coverage by 0.0339%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##             master     #10339       +/-   ##
===============================================
- Coverage   77.6992%   77.6652%   -0.034%     
===============================================
  Files           411        411               
  Lines         85441      85405       -36     
===============================================
- Hits          66387      66330       -57     
- Misses        14095      14111       +16     
- Partials       4959       4964        +5
@codecov

This comment has been minimized.

Copy link

commented May 4, 2019

Codecov Report

Merging #10339 into master will decrease coverage by 0.0114%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10339        +/-   ##
================================================
- Coverage   77.6767%   77.6652%   -0.0115%     
================================================
  Files           411        411                
  Lines         85440      85405        -35     
================================================
- Hits          66367      66330        -37     
+ Misses        14112      14111         -1     
- Partials       4961       4964         +3
@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

/run-all-tests

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

/run-unit-test

@siddontang

This comment has been minimized.

Copy link
Member

commented May 4, 2019

do we need to set tidb_retry_limit to 0 by default later?

@siddontang

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Please test this in jepsen @overvenus

@winkyao
Copy link
Member

left a comment

Could you use failpoint to add some tests ?

@shenli

This comment has been minimized.

Copy link
Member

commented May 4, 2019

@siddontang Yes, I think so. But for autocommit txn, we could do the retry.

@shenli

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Should we merge this PR after #10266 ? I think this PR will make user's query easier to fail.

@shenli shenli added the priority/P1 label May 4, 2019

@winkyao

This comment has been minimized.

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

Could you use failpoint to add some tests?

I think it is not necessary, this pr actually reverts something to it used to be.

Is this PR duplicated with https://github.com/pingcap/tidb/pull/10266/files#diff-928cb614ae092ed4c6f349f4c4c991e4R408?

It is part of #10266, I think it is better to split it out of that pr.

@winkyao

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

Should we merge this PR after #10266 ? I think this PR will make user's query easier to fail.

I think no need to do it. tidb_disable_txn_auto_retry is not suggested to our users to use it. It works for write conflict recently, it didn't retry all errors for a long time. @shenli

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

do we need to set tidb_retry_limit to 0 by default later?

I think we could set tidb_disable_txn_auto_retry to 1 by default later. Then the auto-commit transactions could retry. @siddontang

@siddontang

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Then the auto-commit transactions could retry

@jackysp so we still need to improve other retires like PD crashed later?

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

Then the auto-commit transactions could retry

@jackysp so we still need to improve other retires like PD crashed later?

We could improve the backoff in TiDB.

@disksing

This comment has been minimized.

Copy link
Member

commented May 5, 2019

LGTM

@winkyao

winkyao approved these changes May 5, 2019

Copy link
Member

left a comment

LGTM

@lysu

lysu approved these changes May 5, 2019

Copy link
Member

left a comment

LGTM

@jackysp jackysp merged commit 71d15aa into pingcap:master May 5, 2019

4 of 6 checks passed

ci/circleci CircleCI is running your tests
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job is running.
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

benpigchu added a commit to benpigchu/tidb that referenced this pull request May 7, 2019

make tidb_disable_txn_auto_retry as it used to be (pingcap#10339)
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

@jackysp jackysp deleted the jackysp:revert_retry branch May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.