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

*: add a variable to control the back off time and disable txn auto retry by default #10266

Merged
merged 7 commits into from May 8, 2019

Conversation

@jackysp
Copy link
Member

commented Apr 25, 2019

What problem does this PR solve?

If TiKV doesn't return a retryable error, TiDB should not retry the transaction. But we need a way to control the back off time to avoid the SQL failing when the PD/TiKV is not available.

What is changed and how it works?

Add a variable to control the back off time, and disable txn auto retry by default.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity
@jackysp

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

/run-all-tests

@jackysp

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

/rebuild

1 similar comment
@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

/rebuild

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

/run-all-tests

@codecov

This comment has been minimized.

Copy link

commented Apr 29, 2019

Codecov Report

Merging #10266 into master will decrease coverage by 0.0138%.
The diff coverage is 77.7777%.

@@               Coverage Diff                @@
##             master     #10266        +/-   ##
================================================
- Coverage   77.8343%   77.8204%   -0.0139%     
================================================
  Files           410        410                
  Lines         84306      84330        +24     
================================================
+ Hits          65619      65626         +7     
- Misses        13797      13807        +10     
- Partials       4890       4897         +7
@codecov

This comment has been minimized.

Copy link

commented Apr 29, 2019

Codecov Report

Merging #10266 into master will decrease coverage by 0.0102%.
The diff coverage is 76.923%.

@@               Coverage Diff                @@
##             master     #10266        +/-   ##
================================================
- Coverage   77.3234%   77.3131%   -0.0103%     
================================================
  Files           412        412                
  Lines         85542      85543         +1     
================================================
- Hits          66144      66136         -8     
- Misses        14383      14387         +4     
- Partials       5015       5020         +5
@disksing

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I think you can split it into 2 PRs. One for the weight variable, one for refactor write conflict error.

@jackysp

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

It is not a big PR. I think it is not necessary.

@AndreMouche
Copy link
Member

left a comment

Should we split this PR into two? It seems we both add tidb_back_off_weight and fix the retryableMark for network error?

Show resolved Hide resolved kv/variables.go Outdated
@@ -241,6 +241,9 @@ func (b *Backoffer) WithVars(vars *kv.Variables) *Backoffer {
if vars != nil {
b.vars = vars
}
if math.MaxInt32/b.vars.BackOffWeight >= b.maxSleep {
b.maxSleep *= b.vars.BackOffWeight

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Apr 29, 2019

Member

So all backoff will use this weight? How about specifying only prewrite,commit,get_tso which will only be used in 2pc?

This comment has been minimized.

Copy link
@jackysp

jackysp Apr 29, 2019

Author Member

What are the benefits of doing this?

This comment has been minimized.

Copy link
@jackysp

jackysp Apr 29, 2019

Author Member

I mean, if there is a physical failure, the need to change the backoff should be similar. Still keeping other values fixed, which may make this parameter very useless.

jackysp added some commits Apr 25, 2019

change the default value to 2
Signed-off-by: Yu Shuaipeng <jackysp@gmail.com>
address comments
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

@jackysp jackysp force-pushed the jackysp:control_backoff branch from 47fcc9f to f525557 May 5, 2019

@jackysp jackysp changed the title *: add a variable to control the back off time *: add a variable to control the back off time and disable txn auto retry by default May 5, 2019

disable auto retry by default
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

@jackysp jackysp force-pushed the jackysp:control_backoff branch from f22e526 to 4128691 May 5, 2019

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

/run-all-tests

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

/run-common-test tidb-test=pr/800

1 similar comment
@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

/run-common-test tidb-test=pr/800

@AndreMouche
Copy link
Member

left a comment

LGTM

@jackysp jackysp removed the status/DNM label May 6, 2019

@jackysp jackysp requested a review from lysu May 6, 2019

@jackysp

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@lysu lysu added the status/LGT1 label May 6, 2019

@lysu

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@jackysp Please add more description or comment about background & how-to-use for BackOffWeight, it hard to understand, why we should

	if math.MaxInt32/b.vars.BackOffWeight >= b.maxSleep {
		b.maxSleep *= b.vars.BackOffWeight
	}

and how and when to choose right BackOffWeight value for new viewer like me :D

add comments
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@lysu
Copy link
Member

left a comment

LGTM

@lysu lysu added status/LGT2 and removed status/LGT1 labels May 7, 2019

@disksing

This comment has been minimized.

Copy link
Member

commented May 7, 2019

LGTM

@disksing disksing added status/LGT3 and removed status/LGT2 labels May 7, 2019

@disksing

This comment has been minimized.

Copy link
Member

commented May 7, 2019

/run-all-tests

@@ -18,6 +18,9 @@ type Variables struct {
// BackoffLockFast specifies the LockFast backoff base duration in milliseconds.
BackoffLockFast int

// BackOffWeight specifies the weight of the max back off time duration.
BackOffWeight int

This comment has been minimized.

Copy link
@zz-jason

zz-jason May 7, 2019

Member

backoff weight is a little confusion, how about directly set the max backoff time?

This comment has been minimized.

Copy link
@jackysp

jackysp May 7, 2019

Author Member

There are many kinds of backoffs. Maybe set them separately in the future.

@jackysp jackysp merged commit d8589df into pingcap:master May 8, 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
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.