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

tikv: don't collapse batch resolve locks #17025

Merged
merged 2 commits into from
May 8, 2020

Conversation

youjiali1995
Copy link
Contributor

@youjiali1995 youjiali1995 commented May 7, 2020

Signed-off-by: youjiali1995 zlwgx1023@gmail.com

What problem does this PR solve?

Problem Summary:
#16838 collapses the same resolve lock requests. However, it uses RegionId + StartVersion as the key:

key := strconv.FormatUint(resolveLock.Context.RegionId, 10) + "-" + strconv.FormatUint(resolveLock.StartVersion, 10)

When doing GC, GC worker calls BatchResolveLocks whose StartVersion is always 0. It results in unexpected collapses:

req := tikvrpc.NewRequest(tikvrpc.CmdResolveLock, &kvrpcpb.ResolveLockRequest{TxnInfos: listTxnInfos})

What is changed and how it works?

What's Changed:
Don't collapse batch resolve locks.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit tests

Side effects

Release note

  • No release note

@youjiali1995
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member

jackysp commented May 7, 2020

How did you find this bug? @youjiali1995

@youjiali1995
Copy link
Contributor Author

How did you find this bug? @youjiali1995

Green GC test always fails after upgrading dependency of it to latest release-4.0.

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #17025 into master will decrease coverage by 0.0988%.
The diff coverage is 100.0000%.

@@               Coverage Diff                @@
##             master     #17025        +/-   ##
================================================
- Coverage   80.0804%   79.9815%   -0.0989%     
================================================
  Files           510        510                
  Lines        139305     138797       -508     
================================================
- Hits         111556     111012       -544     
- Misses        18788      18817        +29     
- Partials       8961       8968         +7     

@lysu lysu added the status/LGT1 Indicates that a PR has LGTM 1. label May 7, 2020
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member

jackysp commented May 8, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 8, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

@youjiali1995 merge failed.

@MyonKeminta
Copy link
Contributor

Does this affect data correctness? If so, it will affect legacy resolvelocks mode too

@youjiali1995
Copy link
Contributor Author

Does this affect data correctness? If so, it will affect legacy resolvelocks mode too

Yes. But It is not released in any version. @MyonKeminta

@youjiali1995
Copy link
Contributor Author

/run-unit-test

@jackysp jackysp merged commit eab85ee into pingcap:master May 8, 2020
@youjiali1995 youjiali1995 deleted the fix-collapse-resolve-lock branch May 8, 2020 05:40
sre-bot added a commit to sre-bot/tidb that referenced this pull request May 8, 2020
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

Co-authored-by: pingcap-github-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

cherry pick to release-4.0 in PR #17033

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/store status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants