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.

Sign up for GitHub

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

Jump to bottom

tikv: remove the update leader backoff (#17541) #19764

Merged
merged 8 commits into from
Sep 21, 2020
Merged

tikv: remove the update leader backoff (#17541) #19764

merged 8 commits into from
Sep 21, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #17541 to release-3.0

Signed-off-by: Liqi Geng [email protected]

What problem does this PR solve?

Issue Number: tikv/tikv#7941

Problem Summary:
When a down TiKV recovers, PD will transfer many leaders to this TiKV. I found it will cause that QPS recovers so slowly because of TiDB backoff strategy.
If a region's leader has transferred, the request to this region may get a NotLeader error with a correct leader in most cases. Then TiDB will backoff at least 1ms(then 2,4,8,10ms) which is very wasteful.
In tikv/tikv#7941, it's showed that the QPS recover time is reduced to 2-4min if we remove the update leader backoff, while the original time is 10min.

What is changed and how it works?

What's Changed:
Remove the update leader backoff.

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Side effects

No.

Release note

  • Mitigate the impact of failure recovery on QPS.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot mentioned this pull request Sep 3, 2020
Merged
@ti-srebot
Copy link
Contributor Author

@gengliqi please accept the invitation then you can push to the cherry-pick pull requests.
https://github.com/ti-srebot/tidb/invitations

zz-jason
zz-jason reviewed Sep 5, 2020
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

@gengliqi please resolve the conflicts.

@ti-srebot
Copy link
Contributor Author

@zz-jason, @lysu, @disksing, @sre-bot, PTAL.

@lysu
Copy link
Contributor

lysu commented Sep 7, 2020

@gengliqi please resolve the conflicts.

@gengliqi
Copy link
Contributor

gengliqi commented Sep 8, 2020

@gengliqi please resolve the conflicts.

Resolved, PTAL.

  1. unit-test failed due to data race in TestSyncerSimple which should be race detected during execution of test #16449
  2. check-dev failed but I don‘t know why.😂

@lysu
Copy link
Contributor

lysu commented Sep 8, 2020

/run-all-tests

lzmhhh123
lzmhhh123 reviewed Sep 8, 2020
View reviewed changes
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 8, 2020
@lysu
Copy link
Contributor

lysu commented Sep 8, 2020

/run-unit-test

@ti-srebot
Copy link
Contributor Author

@zz-jason, @lzmhhh123, @lysu, @disksing, @sre-bot, PTAL.

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 10, 2020
@lysu
Copy link
Contributor

lysu commented Sep 11, 2020

/run-unit-test

@ti-srebot
Copy link
Contributor Author

@zz-jason, @lzmhhh123, @disksing, @lysu, @sre-bot, PTAL.

@crazycs520
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@zz-jason, @lzmhhh123, @disksing, @lysu, @sre-bot, PTAL.

@disksing
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor Author

Sorry @disksing, you don't have permission to trigger auto merge event on this branch.
The version releasement is in progress.

@ti-srebot
Copy link
Contributor Author

@zz-jason, @lzmhhh123, @disksing, @lysu, @sre-bot, PTAL.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor Author

Sorry @zz-jason, you don't have permission to trigger auto merge event on this branch.
The version releasement is in progress.

@jebter
Copy link

jebter commented Sep 21, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 21, 2020
@zhouqiang-cl
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@jebter
Copy link

jebter commented Sep 21, 2020

/run-integration-common-test

@jebter
Copy link

jebter commented Sep 21, 2020

/run-unit-test

@gengliqi gengliqi added this to the 3.0.19 milestone Sep 21, 2020
@jebter
Copy link

jebter commented Sep 21, 2020

/run-unit-test

@zhouqiang-cl zhouqiang-cl merged commit 7f2b135 into pingcap:release-3.0 Sep 21, 2020
@wjhuang2016 wjhuang2016 mentioned this pull request Sep 24, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@lzmhhh123 lzmhhh123 lzmhhh123 approved these changes

@disksing disksing disksing approved these changes

@lysu lysu Awaiting requested review from lysu

@sre-bot sre-bot Awaiting requested review from sre-bot

Assignees

@gengliqi gengliqi

Labels
component/tikv status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement. type/3.0-cherry-pick
Projects
None yet
Milestone
v3.0.19
Development

Successfully merging this pull request may close these issues.
10 participants
@ti-srebot @lysu @gengliqi @crazycs520 @disksing @zz-jason @jebter @zhouqiang-cl @lzmhhh123 @wjhuang2016