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

gc_worker: reduce GC scan locks when meeting region cache miss #18385

Merged
merged 6 commits into from
Jul 30, 2020
Merged

gc_worker: reduce GC scan locks when meeting region cache miss #18385

merged 6 commits into from
Jul 30, 2020

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Jul 6, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

currently, gc worker will rescan locks when meet region cache miss error which makes slow scan job become more slow.

What is changed and how it works?

What's Changed, How it Works:

when region cache miss error(current is a fake epochNotMatch), it just indicates "maybe region has changed and we need to reload region from PD", we can continue resolve scan lock result if all lock still in that region.

this can help situations like:

  • lock and region hasn't changed but region cache expired
  • lock's region's key range didn't change but maybe add new peer(id is same but confVer or ver changed)
  • all lock has changed from region A to region B

but it didn't help the situation that:

  • lock's region has been split into two region

maybe we can improve it later(to avoid complex modification)

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • n/a

Release note

  • Reduce GC scan locks when meeting region cache miss

This change is Reviewable

@lysu lysu added type/enhancement The issue or PR belongs to an enhancement. component/GC labels Jul 6, 2020
@lysu lysu changed the title gcworker: reduce GC scan locks when meeting region cache miss gc_worker: reduce GC scan locks when meeting region cache miss Jul 6, 2020
@codecov
Copy link

codecov bot commented Jul 6, 2020 •
edited
Loading

Codecov Report

Merging #18385 into master will decrease coverage by 0.0165%.
The diff coverage is 62.0689%.

@@               Coverage Diff                @@
##             master     #18385        +/-   ##
================================================
- Coverage   79.1461%   79.1296%   -0.0166%     
================================================
  Files           548        547         -1     
  Lines        147949     147726       -223     
================================================
- Hits         117096     116895       -201     
+ Misses        21369      21361         -8     
+ Partials       9484       9470        -14

@lysu
Copy link
Contributor Author

lysu commented Jul 6, 2020

/run-all-tests

@lysu lysu added this to the v3.0.17 milestone Jul 6, 2020
youjiali1995
youjiali1995 reviewed Jul 8, 2020
View reviewed changes
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

Rest lgtm. But it's a little strange that don't solve the problem completely.

store/tikv/gcworker/gc_worker.go Outdated Show resolved Hide resolved
store/tikv/gcworker/gc_worker_test.go Outdated Show resolved Hide resolved
store/tikv/gcworker/gc_worker_test.go Outdated Show resolved Hide resolved
@ti-srebot
Copy link
Contributor

@youjiali1995, @MyonKeminta, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor

@youjiali1995, @MyonKeminta, PTAL.

@lysu lysu modified the milestones: v3.0.17, v4.0.3 Jul 14, 2020
youjiali1995
youjiali1995 reviewed Jul 14, 2020
View reviewed changes
Copy link
Contributor

@youjiali1995 youjiali1995 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 Jul 14, 2020
@winoros winoros modified the milestones: v4.0.3, v4.0.4 Jul 15, 2020
@ti-srebot
Copy link
Contributor

@youjiali1995, @coocood, @jackysp, @MyonKeminta, PTAL.

MyonKeminta
MyonKeminta reviewed Jul 16, 2020
View reviewed changes
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

rest LGTM

store/tikv/gcworker/gc_worker.go Outdated Show resolved Hide resolved
@ti-srebot
Copy link
Contributor

@youjiali1995, @MyonKeminta, @coocood, @jackysp, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor

@youjiali1995, @MyonKeminta, @coocood, @jackysp, PTAL.

@MyonKeminta
Copy link
Contributor

Can we continue this or just merge this? @youjiali1995 @lysu

@ti-srebot
Copy link
Contributor

@youjiali1995, @MyonKeminta, @coocood, @jackysp, PTAL.

@lysu lysu dismissed stale reviews from ti-srebot and jackysp via 7cc9877 July 30, 2020 05:12
@lysu
Copy link
Contributor Author

lysu commented Jul 30, 2020

/run-all-tests

@jackysp jackysp merged commit 43c2d30 into pingcap:master Jul 30, 2020
@lysu lysu deleted the dev-fix-gc-work-re-scan branch July 30, 2020 05:40
@ti-srebot ti-srebot mentioned this pull request Jul 30, 2020
Merged
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 30, 2020
@lysu @ti-srebot
cherry pick pingcap#18385 to release-3.0
af27f1f 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #18874

@ti-srebot ti-srebot mentioned this pull request Jul 30, 2020
Merged
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 30, 2020
@lysu @ti-srebot
cherry pick pingcap#18385 to release-3.1
2c34aaa 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot
Copy link
Contributor

cherry pick to release-3.1 in PR #18875

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 30, 2020
@lysu @ti-srebot
cherry pick pingcap#18385 to release-4.0
8977226 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jul 30, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18876

jackysp pushed a commit that referenced this pull request Jul 30, 2020
@ti-srebot
cherry pick #18385 to release-4.0 (#18876)
6055d8b
ti-srebot added a commit that referenced this pull request Aug 9, 2020
@ti-srebot
gc_worker: reduce GC scan locks when meeting region cache miss (#18385)…
393c711 
… (#18875)

Signed-off-by: ti-srebot <[email protected]>
ti-srebot added a commit that referenced this pull request Aug 26, 2020
@ti-srebot
gc_worker: reduce GC scan locks when meeting region cache miss (#18385)…
dae7cba 
… (#18874)

Signed-off-by: ti-srebot <[email protected]>
@youjiali1995 youjiali1995 mentioned this pull request Jan 7, 2021
Closed
@lysu lysu mentioned this pull request Jan 7, 2021
Merged
This was referenced Jan 7, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youjiali1995 youjiali1995 youjiali1995 left review comments

@MyonKeminta MyonKeminta MyonKeminta left review comments

@jackysp jackysp jackysp approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

@coocood coocood Awaiting requested review from coocood

Assignees
No one assigned
Labels
component/GC 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.
Projects
None yet
Milestone
v4.0.5
Development

Successfully merging this pull request may close these issues.
7 participants
@lysu @ti-srebot @MyonKeminta @jackysp @youjiali1995 @winoros @imtbkcat