store/tikv: refactor the ResolveLocks() function for large transaction's implementation #11999

tiancaiamao · 2019-09-03T05:11:24Z

What problem does this PR solve?

After introducing the large transaction, the true lock TTL information is stored in the primary lock, and the TTL may change.
This commit updates the resolve lock method, using the new kvproto API.

What is changed and how it works?

Tiny refactor to adapt the new kvproto API.
~~Blocked by #11974 which implements the CheckTxnStatus API, please review that one first.~~

Check List

Tests

No code

…n's implementation After introducing the large transaction, the true lock TTL information is stored in the primary lock, and the TTL may change. This commit updates the resolve lock method, using the new kvproto API

codecov · 2019-09-11T16:17:06Z

Codecov Report

Merging #11999 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11999   +/-   ##
===========================================
  Coverage   79.9206%   79.9206%           
===========================================
  Files           461        461           
  Lines        104087     104087           
===========================================
  Hits          83187      83187           
  Misses        14795      14795           
  Partials       6105       6105

tiancaiamao · 2019-09-11T16:31:23Z

PTAL @coocood @lysu @MyonKeminta

coocood · 2019-09-12T02:58:37Z

It's actually blocked by tikv/tikv#5390 for integration test.

MyonKeminta · 2019-09-12T05:36:32Z

tikv/tikv#5390 is ready for reviewing, though I'll add some more test to it

store/tikv/2pc.go

tiancaiamao · 2019-09-23T06:04:32Z

~~I'll update this PR after merging #12212~~

store/tikv/lock_resolver.go

tiancaiamao · 2019-10-08T08:19:34Z

/run-all-tests

tiancaiamao · 2019-10-08T09:11:50Z

/run-integration-common-test tikv=pr/5390

tiancaiamao · 2019-10-08T09:36:54Z

/run-all-tests tikv=pr/5390

tiancaiamao · 2019-10-08T09:41:55Z

PTAL @MyonKeminta @youjiali1995 @coocood

store/tikv/lock_resolver.go

tiancaiamao · 2019-10-10T01:52:16Z

PTAL @coocood @MyonKeminta

tiancaiamao · 2019-10-12T09:51:50Z

I find there so many repeated codes like this

	for {
		loc, err := lr.store.GetRegionCache().LocateKey(bo, primary)
		if err != nil {
			return status, errors.Trace(err)
		}
		resp, err := lr.store.SendReq(bo, req, loc.Region, readTimeoutShort)
		if err != nil {
			return status, errors.Trace(err)
		}
		regionErr, err := resp.GetRegionError()
		if err != nil {
			return status, errors.Trace(err)
		}

and I will clean up them after this PR

tiancaiamao · 2019-10-12T09:57:30Z

getTxnStatusFromLock now cleanup primary lock correctly
The test is updated to cover secondary lock and lock.TTL = 0

@MyonKeminta @coocood @sticnarf @youjiali1995

store/tikv/lock_resolver.go

youjiali1995 · 2019-10-12T10:54:36Z

LGTM

store/tikv/lock_resolver.go

sticnarf

The rest LGTM

MyonKeminta · 2019-10-14T04:02:11Z

/run-all-tests tikv=pr/5390

sre-bot · 2019-10-14T05:27:35Z

/run-all-tests

sre-bot · 2019-10-14T05:32:33Z

@tiancaiamao merge failed.

tiancaiamao · 2019-10-14T05:35:11Z

/run-integration-common-test

tiancaiamao · 2019-10-14T05:39:04Z

/run-unit-test

zyxbest · 2019-10-14T05:45:00Z

/run-unit-test

tiancaiamao · 2019-10-14T05:45:56Z

That's weird.

[2019-10-14T05:40:07.994Z] ----------------------------------------------------------------------
[2019-10-14T05:40:07.994Z] FAIL: github.com/pingcap/tidb@/config/config_test.go:37: testConfigSuite.TestConfig
[2019-10-14T05:40:07.994Z] 
[2019-10-14T05:40:07.994Z] github.com/pingcap/tidb@/config/config_test.go:50:
[2019-10-14T05:40:07.994Z]     ...open github.com/pingcap/tidb@/config/config_test.go: no such file or directory
[2019-10-14T05:40:07.994Z] ... value *os.PathError = &os.PathError{Op:"open", Path:"github.com/pingcap/tidb@/config/config.toml", Err:0x2} ("open github.com/pingcap/tidb@/config/config.toml: no such file or directory")
[2019-10-14T05:40:07.994Z] 
[2019-10-14T05:40:07.994Z] 
[2019-10-14T05:40:07.994Z] ----------------------------------------------------------------------

MyonKeminta · 2019-10-16T03:40:19Z

store/tikv/lock_resolver.go

+
+		if status.ttl > 0 {
+			// Do not clean lock that is not expired.
+			continue


This function need to resolve all locks in locks. If some of the locks can't be resolved, this function shouldn't return true.

Then large txn would block GC...

To continue GC anyway, you should resolve all the locks before cleaning up the data. Otherwise not-committed secondaries may miss their primary.

You may remove these locks regardless of their TTL (of course, primary first), but you can never leave these locks there.

We may need to update the CmdGCRequest instead of simply leave the lock here.
@coocood

How will GC's logic be? By the way, GC Request is not used by default now.

…n's implementation (pingcap#11999) After introducing the large transaction, the true lock TTL information is stored in the primary lock, and the TTL may change. This commit updates the resolve lock method, using the new kvproto CheckTxnStatus API

tiancaiamao added 3 commits September 2, 2019 12:04

*: implement the CheckTxnStatus API for the large transaction

b1c67f0

make check

52c1d29

tiancaiamao added the component/tikv label Sep 3, 2019

Merge branch 'master' into resolve-lock

a4e8c39

tiny cleanup

4c6c5c2

tiancaiamao mentioned this pull request Sep 17, 2019

store: update kvrpc.Cleanup proto and change its behaviour #12212

Merged

youjiali1995 reviewed Sep 17, 2019

View reviewed changes

store/tikv/2pc.go Outdated Show resolved Hide resolved

tiancaiamao added 5 commits September 27, 2019 20:33

Merge branch 'master' into resolve-lock

db5d7a7

use CheckTxnStatus to implement getTxnStatus

ce3b782

update comment

5b0a56c

Merge branch 'master' into resolve-lock

d212fbf

resolve lock for GC

7754fbd

tiancaiamao commented Oct 8, 2019

View reviewed changes

store/tikv/lock_resolver.go Show resolved Hide resolved

fix build

edc03ff

fix integrated test

3c96f57

tiancaiamao added the status/all tests passed label Oct 8, 2019

coocood reviewed Oct 8, 2019

View reviewed changes

store/tikv/lock_resolver.go Outdated Show resolved Hide resolved

tiancaiamao requested a review from coocood October 10, 2019 01:51

address comment

6da84f7

address comment

3e1f4a7

tiancaiamao removed the status/WIP label Oct 12, 2019

make lint happy

4261614

youjiali1995 reviewed Oct 12, 2019

View reviewed changes

store/tikv/lock_resolver.go Outdated Show resolved Hide resolved

tiancaiamao added 2 commits October 12, 2019 19:25

address comment

f45db06

Merge branch 'master' into resolve-lock

cf2127c

sticnarf reviewed Oct 12, 2019

View reviewed changes

store/tikv/lock_resolver.go Outdated Show resolved Hide resolved

sticnarf reviewed Oct 12, 2019

View reviewed changes

tiancaiamao added 2 commits October 12, 2019 20:10

make CI stable

d0f6fc5

address comment

f2a887f

coocood approved these changes Oct 14, 2019

View reviewed changes

coocood added the status/can-merge Indicates a PR has been approved by a committer. label Oct 14, 2019

Merge branch 'master' into resolve-lock

2a1d71d

tiancaiamao merged commit eda7656 into pingcap:master Oct 14, 2019

tiancaiamao deleted the resolve-lock branch October 14, 2019 05:54

MyonKeminta reviewed Oct 16, 2019

View reviewed changes

tiancaiamao mentioned this pull request Oct 17, 2019

store/tikv: resolve all locks no matter they're expired or not in BatchResolveLocks #12784

Merged

csuzhangxc mentioned this pull request Dec 9, 2020

pump: lost transaction when ttl exceeded pingcap/tidb-binlog#1024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store/tikv: refactor the ResolveLocks() function for large transaction's implementation #11999

store/tikv: refactor the ResolveLocks() function for large transaction's implementation #11999

tiancaiamao commented Sep 3, 2019 •

edited

Loading

codecov bot commented Sep 11, 2019 •

edited

Loading

tiancaiamao commented Sep 11, 2019

coocood commented Sep 12, 2019

MyonKeminta commented Sep 12, 2019

tiancaiamao commented Sep 23, 2019 •

edited

Loading

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 10, 2019

tiancaiamao commented Oct 12, 2019

tiancaiamao commented Oct 12, 2019

youjiali1995 commented Oct 12, 2019

sticnarf left a comment

MyonKeminta commented Oct 14, 2019

sre-bot commented Oct 14, 2019

sre-bot commented Oct 14, 2019

tiancaiamao commented Oct 14, 2019

tiancaiamao commented Oct 14, 2019

zyxbest commented Oct 14, 2019

tiancaiamao commented Oct 14, 2019

MyonKeminta Oct 16, 2019

tiancaiamao Oct 16, 2019

MyonKeminta Oct 16, 2019

MyonKeminta Oct 16, 2019 •

edited

Loading

tiancaiamao Oct 16, 2019

MyonKeminta Oct 17, 2019

store/tikv: refactor the ResolveLocks() function for large transaction's implementation #11999

store/tikv: refactor the ResolveLocks() function for large transaction's implementation #11999

Conversation

tiancaiamao commented Sep 3, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Sep 11, 2019 • edited Loading

Codecov Report

tiancaiamao commented Sep 11, 2019

coocood commented Sep 12, 2019

MyonKeminta commented Sep 12, 2019

tiancaiamao commented Sep 23, 2019 • edited Loading

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 8, 2019

tiancaiamao commented Oct 10, 2019

tiancaiamao commented Oct 12, 2019

tiancaiamao commented Oct 12, 2019

youjiali1995 commented Oct 12, 2019

sticnarf left a comment

Choose a reason for hiding this comment

MyonKeminta commented Oct 14, 2019

sre-bot commented Oct 14, 2019

sre-bot commented Oct 14, 2019

tiancaiamao commented Oct 14, 2019

tiancaiamao commented Oct 14, 2019

zyxbest commented Oct 14, 2019

tiancaiamao commented Oct 14, 2019

MyonKeminta Oct 16, 2019

Choose a reason for hiding this comment

tiancaiamao Oct 16, 2019

Choose a reason for hiding this comment

MyonKeminta Oct 16, 2019

Choose a reason for hiding this comment

MyonKeminta Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

tiancaiamao Oct 16, 2019

Choose a reason for hiding this comment

MyonKeminta Oct 17, 2019

Choose a reason for hiding this comment

tiancaiamao commented Sep 3, 2019 •

edited

Loading

codecov bot commented Sep 11, 2019 •

edited

Loading

tiancaiamao commented Sep 23, 2019 •

edited

Loading

MyonKeminta Oct 16, 2019 •

edited

Loading