store: upgrade the CheckTxnStatus API #13123

tiancaiamao · 2019-11-04T15:28:53Z

What problem does this PR solve?

CheckTxnStatus introduces a non-block read mode.
In this mode, TiDB can ignore the secondary lock TTL check and send the CheckTxnStatus request.

What is changed and how it works?

The 2PC prewrite operation writes the primary and the secondary regions concurrently.
When the CheckTxnStatus request arrives, the primary key lock may be still ongoing (secondary locks exist while the primary lock doesn't).
We can back off and retry CheckTxnStatus in that case.

update kvproto
add a noWait parameter to the ResolveLocks() function
handle TxnNotFound error and retry in getTxnStatusFromLock()
handle the RollbackIfNotExist field of the CheckTxnStatus request in the mocktikv

Check List

Tests

Unit test

Code changes

Has exported function/method change

Side effects

Increased code complexity

CheckTxnStatus introduces a non-block read mode. In this mode, TiDB can ignore the secondary lock TTL check and send the CheckTxnStatus request.

tiancaiamao · 2019-11-04T15:31:55Z

PTAL @coocood @MyonKeminta @cfzjywxk

codecov · 2019-11-04T15:50:30Z

Codecov Report

Merging #13123 into master will decrease coverage by 0.6806%.
The diff coverage is 75.8064%.

@@               Coverage Diff                @@
##             master     #13123        +/-   ##
================================================
- Coverage   80.8733%   80.1926%   -0.6807%     
================================================
  Files           469        469                
  Lines        115012     111519      -3493     
================================================
- Hits          93014      89430      -3584     
- Misses        15054      15197       +143     
+ Partials       6944       6892        -52

coocood · 2019-11-05T03:34:41Z

We don't need noWait parameter, it should always be true.

store/tikv/lock_resolver.go

tiancaiamao · 2019-11-05T09:17:20Z

We don't need noWait parameter, it should always be true.

Removed. @coocood

Note, we have no switch, so after this PR the resolve lock logic is changed.
It affects not only the new large transaction feature but also the old code!

tiancaiamao · 2019-11-05T09:19:42Z

/run-all-tests

store/tikv/lock_resolver.go

tiancaiamao · 2019-11-05T12:01:57Z

store/mockstore/mocktikv/mvcc_leveldb.go

@@ -919,7 +919,7 @@ func rollbackKey(db *leveldb.DB, batch *leveldb.Batch, key []byte, startTS uint6
 	return nil
 }

-func rollbackLock(batch *leveldb.Batch, lock mvccLock, key []byte, startTS uint64) error {


This parameter is not used in the function, so I remove it

tiancaiamao · 2019-11-05T12:13:13Z

PTAL @coocood @MyonKeminta @cfzjywxk

coocood · 2019-11-05T12:22:43Z

store/tikv/lock_resolver.go

-	return lr.getTxnStatus(bo, l.TxnID, l.Primary, callerStartTS, currentTS)
+
+	rollbackIfNotExist := false
+	for {


There is already a for loop in getTxnStatus why not handle retry in getTxnStatus directly?

To make the unit test easier.

store/tikv/lock_resolver.go

coocood · 2019-11-05T12:34:05Z

store/tikv/lock_test.go

+	}
+	// Call getTxnStatusFromLock to cover the retry logic.
+	status, err := resolver.getTxnStatusFromLock(bo, lock, currentTS)
+	c.Assert(err, IsNil)


At this time, the lock may still not written, we can only be sure it is written after <-errCh.

The lock may still not written, and getTxnStatusFromLock should retry in that case, so we cover the retry logic.
The lock TTL is set to 10s, long enough for the goroutine to finish, the result should be stable.

or we add some sleep before errCh <- xxx to ensure covering the retry path

or we add some sleep before errCh <- xxx to ensure covering the retry path

So as to make the CI slower?

tiancaiamao · 2019-11-05T16:14:32Z

/run-all-tests

tiancaiamao · 2019-11-06T01:53:25Z

/rebuild

coocood · 2019-11-06T02:31:02Z

LGTM

coocood · 2019-11-06T02:34:13Z

@MyonKeminta @youjiali1995 PTAL

youjiali1995

LGTM

MyonKeminta

Mostly LGTM

MyonKeminta · 2019-11-06T05:01:33Z

store/mockstore/mocktikv/mock_tikv_test.go

@@ -663,22 +663,22 @@ func (s *testMVCCLevelDB) TestErrors(c *C) {
 func (s *testMVCCLevelDB) TestCheckTxnStatus(c *C) {
 	s.mustPrewriteWithTTLOK(c, putMutations("pk", "val"), "pk", 5, 666)

-	ttl, commitTS, err := s.store.CheckTxnStatus([]byte("pk"), 5, 0, 666)
+	ttl, commitTS, err := s.store.CheckTxnStatus([]byte("pk"), 5, 0, 666, false)


I think you need to cover the case where the last parameter is true.

store/tikv/lock_resolver.go

MyonKeminta · 2019-11-06T06:47:04Z

store/tikv/lock_resolver.go

+		if _, ok := errors.Cause(err).(txnNotFoundErr); !ok {
+			return TxnStatus{}, err
+		}


I think here it needs comments to explain it.

MyonKeminta

LGTM

tiancaiamao · 2019-11-07T08:00:35Z

/run-all-tests

tiancaiamao · 2019-11-07T08:03:30Z

/run-all-tests

tiancaiamao · 2019-11-07T08:10:31Z

/run-unit-test

coocood

LGTM

tiancaiamao · 2019-11-07T09:21:06Z

/run-unit-test

tiancaiamao · 2019-11-07T11:05:20Z

/run-all-tests

CheckTxnStatus introduces a non-block read mode. In this mode, TiDB can ignore the secondary lock TTL check and send the CheckTxnStatus request.

tiancaiamao added 2 commits November 4, 2019 23:04

store: upgrade the CheckTxnStatus API

6181920

CheckTxnStatus introduces a non-block read mode. In this mode, TiDB can ignore the secondary lock TTL check and send the CheckTxnStatus request.

Merge branch 'master' into update-check-txn-status

576c051

tiancaiamao added sig/transaction SIG:Transaction component/tikv labels Nov 4, 2019

make golint happy

c0285ba

cfzjywxk reviewed Nov 5, 2019

View reviewed changes

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

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

tiancaiamao added 2 commits November 5, 2019 17:06

address comment

e7aad0d

Merge branch 'master' into update-check-txn-status

8da07bc

coocood reviewed Nov 5, 2019

View reviewed changes

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

tiancaiamao added 2 commits November 5, 2019 19:41

address comment

8106b33

write the rollback key in mocktikv

386ca77

coocood reviewed Nov 5, 2019

View reviewed changes

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

tiny clean up

bba8d04

tiancaiamao commented Nov 5, 2019

View reviewed changes

coocood reviewed Nov 5, 2019

View reviewed changes

store/tikv/lock_resolver.go Show resolved Hide resolved

coocood reviewed Nov 5, 2019

View reviewed changes

tiancaiamao added 6 commits November 5, 2019 21:08

address comment

fbc600f

make CI stable

5fb661a

address comment

bbd42e3

fix a data race

e77e17b

fix data race

64721b2

Merge branch 'master' into update-check-txn-status

c01bc0a

youjiali1995 reviewed Nov 6, 2019

View reviewed changes

MyonKeminta reviewed Nov 6, 2019

View reviewed changes

address comment

d4e192a

MyonKeminta reviewed Nov 6, 2019

View reviewed changes

tiancaiamao added 2 commits November 7, 2019 15:47

make CI stable

0f3ae0f

Merge branch 'master' into update-check-txn-status

913e29d

coocood previously approved these changes Nov 7, 2019

View reviewed changes

try to make CI stable

1aa8f9b

tiancaiamao dismissed coocood’s stale review via 1aa8f9b November 7, 2019 11:03

tiancaiamao added the status/all tests passed label Nov 7, 2019

coocood approved these changes Nov 7, 2019

View reviewed changes

tiancaiamao added the require-LGT3 Indicates that the PR requires three LGTM. label Nov 7, 2019

coocood merged commit 60d4291 into pingcap:master Nov 7, 2019

tiancaiamao added status/LGT3 The PR has already had 3 LGTM. and removed require-LGT3 Indicates that the PR requires three LGTM. labels Nov 7, 2019

tiancaiamao deleted the update-check-txn-status branch November 7, 2019 11:09

tiancaiamao mentioned this pull request Nov 8, 2019

session,store/tikv: make CI more stable #13303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store: upgrade the CheckTxnStatus API #13123

store: upgrade the CheckTxnStatus API #13123

tiancaiamao commented Nov 4, 2019 •

edited

tiancaiamao commented Nov 4, 2019

codecov bot commented Nov 4, 2019 •

edited

coocood commented Nov 5, 2019

tiancaiamao commented Nov 5, 2019

tiancaiamao commented Nov 5, 2019

tiancaiamao Nov 5, 2019

tiancaiamao commented Nov 5, 2019

coocood Nov 5, 2019

tiancaiamao Nov 5, 2019

coocood Nov 5, 2019

tiancaiamao Nov 5, 2019

cfzjywxk Nov 5, 2019

tiancaiamao Nov 6, 2019

tiancaiamao commented Nov 5, 2019

tiancaiamao commented Nov 6, 2019

coocood commented Nov 6, 2019

coocood commented Nov 6, 2019

youjiali1995 left a comment

MyonKeminta left a comment

MyonKeminta Nov 6, 2019

tiancaiamao Nov 6, 2019

MyonKeminta Nov 6, 2019

tiancaiamao Nov 6, 2019

MyonKeminta left a comment

tiancaiamao commented Nov 7, 2019

tiancaiamao commented Nov 7, 2019

tiancaiamao commented Nov 7, 2019

coocood left a comment

tiancaiamao commented Nov 7, 2019

tiancaiamao commented Nov 7, 2019

store: upgrade the CheckTxnStatus API #13123

store: upgrade the CheckTxnStatus API #13123

Conversation

tiancaiamao commented Nov 4, 2019 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

tiancaiamao commented Nov 4, 2019

codecov bot commented Nov 4, 2019 • edited

Codecov Report

coocood commented Nov 5, 2019

tiancaiamao commented Nov 5, 2019

tiancaiamao commented Nov 5, 2019

Choose a reason for hiding this comment

tiancaiamao commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Nov 5, 2019

tiancaiamao commented Nov 6, 2019

coocood commented Nov 6, 2019

coocood commented Nov 6, 2019

youjiali1995 left a comment

Choose a reason for hiding this comment

MyonKeminta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta left a comment

Choose a reason for hiding this comment

tiancaiamao commented Nov 7, 2019

tiancaiamao commented Nov 7, 2019

tiancaiamao commented Nov 7, 2019

coocood left a comment

Choose a reason for hiding this comment

tiancaiamao commented Nov 7, 2019

tiancaiamao commented Nov 7, 2019

tiancaiamao commented Nov 4, 2019 •

edited

codecov bot commented Nov 4, 2019 •

edited