store: update kvproto.CheckTxnStatus response #13432

tiancaiamao · 2019-11-13T10:01:40Z

What problem does this PR solve?

CheckTxnStatus should provide such kind of information, so we can find what's happening in the issue #13397

What is changed and how it works?

Record rollback reason in kvproto.CheckTxnStatus response, update the code.

Update:

Now the field name is changed to action, and the CheckTxnStatus response also contains the information whether the minCommitTS is pushed.

Check List

Tests

Unit test

Side effects

Breaking backward compatibility

tiancaiamao · 2019-11-13T10:02:22Z

/run-all-tests tikv=pr/5878

tiancaiamao · 2019-11-13T11:13:04Z

/run-all-tests tikv=pr/5878

codecov · 2019-11-13T11:33:21Z

Codecov Report

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

@@            Coverage Diff            @@
##            master    #13432   +/-   ##
=========================================
  Coverage   80.358%   80.358%           
=========================================
  Files          472       472           
  Lines       115172    115172           
=========================================
  Hits         92550     92550           
  Misses       15397     15397           
  Partials      7225      7225

tiancaiamao · 2019-11-13T11:37:03Z

PTAL @coocood @youjiali1995 @MyonKeminta

tiancaiamao · 2019-11-15T08:21:02Z

PTAL @coocood @youjiali1995 @MyonKeminta

youjiali1995

LGTM

coocood · 2019-11-15T11:17:08Z

LGTM

coocood · 2019-11-15T11:17:44Z

/merge

sre-bot · 2019-11-15T11:20:08Z

/run-all-tests

sre-bot · 2019-11-15T11:22:36Z

@tiancaiamao merge failed.

tiancaiamao · 2019-11-15T14:22:24Z

/run-all-tests

tiancaiamao · 2019-11-18T06:24:50Z

PTAL @coocood @youjiali1995

tiancaiamao · 2019-11-18T06:26:04Z

The kvproto has been updated to include this change: pingcap/kvproto#495
PTAL @coocood @youjiali1995

tiancaiamao · 2019-11-18T06:27:37Z

/run-all-tests

store/tikv/lock_resolver.go

coocood · 2019-11-18T07:01:40Z

store/mockstore/mocktikv/rpc.go

@@ -387,11 +387,11 @@ func (h *rpcHandler) handleKvCheckTxnStatus(req *kvrpcpb.CheckTxnStatusRequest)
 		panic("KvCheckTxnStatus: key not in region")
 	}
 	var resp kvrpcpb.CheckTxnStatusResponse
-	ttl, commitTS, err := h.mvccStore.CheckTxnStatus(req.GetPrimaryKey(), req.GetLockTs(), req.GetCallerStartTs(), req.GetCurrentTs(), req.GetRollbackIfNotExist())
+	ttl, commitTS, rollbackReason, err := h.mvccStore.CheckTxnStatus(req.GetPrimaryKey(), req.GetLockTs(), req.GetCallerStartTs(), req.GetCurrentTs(), req.GetRollbackIfNotExist())


variable name should update

coocood · 2019-11-18T07:02:07Z

store/tikv/lock_test.go

@@ -201,7 +201,7 @@ func (s *testLockSuite) TestGetTxnStatus(c *C) {
 	status, err = s.store.lockResolver.GetTxnStatus(startTS, startTS, []byte("a"))
 	c.Assert(err, IsNil)
 	c.Assert(status.IsCommitted(), IsFalse)
-	c.Assert(status.ttl, Greater, uint64(0))
+	c.Assert(status.ttl, Greater, uint64(0), Commentf("rollback reason:%s", status.action))


log message need update

tiancaiamao · 2019-11-18T07:53:54Z

/run-all-tests

coocood · 2019-11-18T07:54:27Z

store/mockstore/mocktikv/mvcc_leveldb.go

@@ -1072,6 +1079,7 @@ func (mvcc *MVCCLevelDB) CheckTxnStatus(primaryKey []byte, lockTS, callerStartTS
 				// We *must* guarantee the invariance lock.minCommitTS >= callerStartTS + 1
 				if lock.minCommitTS < callerStartTS+1 {
 					lock.minCommitTS = callerStartTS + 1
+					action = kvrpcpb.Action_MinCommitTSPushed


We need to set this action even if minCommitTS is already greater than callerStartTS

Oh, get it!
That information is actually 'could the caller read ignore the lock', rather than 'minCommitTS pushed'

MyonKeminta · 2019-11-18T07:57:53Z

store/tikv/lock_resolver.go

 type TxnStatus struct {
 	ttl      uint64
 	commitTS uint64
+	action   kvrpcpb.Action
 }


Can this definition be moved to a better place in store directory, so those CheckTxnStatus functions can return this struct instead of returning its three fields in a long tuple?

That would involve too many irrelevant changes in this PR

tiancaiamao · 2019-11-18T08:31:58Z

/run-all-tests tikv=pr/5937

tiancaiamao · 2019-11-18T08:46:16Z

/rebuild

MyonKeminta

LGTM

youjiali1995

LGTM

tiancaiamao · 2019-11-18T09:38:29Z

/run-all-tests

tiancaiamao · 2019-11-18T10:03:55Z

/run-unit-test

coocood

LGTM

store: record rollback reason in kvproto.CheckTxnStatus response

7bc8c6f

ekalinin mentioned this pull request Nov 13, 2019

expression: implement vectorized evaluation for builtinSysDateWithFspSig #13332

Merged

fix CI

3014ef9

Merge branch 'master' into rollback-reason

28144de

tiancaiamao added the component/tikv label Nov 13, 2019

tiancaiamao added 2 commits November 13, 2019 23:36

update kvproto

7003818

update kvproto

f08f59e

youjiali1995 mentioned this pull request Nov 15, 2019

txn: add rollback reason for check_txn_status tikv/tikv#5878

Merged

youjiali1995 reviewed Nov 15, 2019

View reviewed changes

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 15, 2019

Merge branch 'master' into rollback-reason

eb9cc30

coocood previously approved these changes Nov 15, 2019

View reviewed changes

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 15, 2019

Merge branch 'master' into rollback-reason

5a47660

tiancaiamao added 2 commits November 18, 2019 13:56

update kvproto

fe11fb9

Merge branch 'master' into rollback-reason

2ff5e70

tiancaiamao dismissed coocood’s stale review via 2ff5e70 November 18, 2019 06:15

tiancaiamao force-pushed the rollback-reason branch from d4ee01b to 2ff5e70 Compare November 18, 2019 06:15

Merge branch 'master' into rollback-reason

90371ad

youjiali1995 reviewed Nov 18, 2019

View reviewed changes

store/tikv/lock_resolver.go Show resolved Hide resolved

address comment

260d155

coocood reviewed Nov 18, 2019

View reviewed changes

address comment

c8b5932

youjiali1995 mentioned this pull request Nov 18, 2019

txn: return the action check_txn_status does tikv/tikv#5937

Merged

address comment

69d7fb6

tiancaiamao changed the title ~~store: record rollback reason in kvproto.CheckTxnStatus response~~ store: update kvproto.CheckTxnStatus response Nov 18, 2019

coocood reviewed Nov 18, 2019

View reviewed changes

MyonKeminta reviewed Nov 18, 2019

View reviewed changes

address comment

df186b8

MyonKeminta reviewed Nov 18, 2019

View reviewed changes

youjiali1995 reviewed Nov 18, 2019

View reviewed changes

Merge branch 'master' into rollback-reason

e4e7403

coocood approved these changes Nov 18, 2019

View reviewed changes

coocood merged commit 8401cbd into pingcap:master Nov 18, 2019

tiancaiamao deleted the rollback-reason branch November 20, 2019 08:44

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019

store: update kvproto.CheckTxnStatus response (pingcap#13432)

272d61a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store: update kvproto.CheckTxnStatus response #13432

store: update kvproto.CheckTxnStatus response #13432

tiancaiamao commented Nov 13, 2019 •

edited

Loading

tiancaiamao commented Nov 13, 2019

tiancaiamao commented Nov 13, 2019

codecov bot commented Nov 13, 2019 •

edited

Loading

tiancaiamao commented Nov 13, 2019

tiancaiamao commented Nov 15, 2019

youjiali1995 left a comment

coocood commented Nov 15, 2019

coocood commented Nov 15, 2019

sre-bot commented Nov 15, 2019

sre-bot commented Nov 15, 2019

tiancaiamao commented Nov 15, 2019

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

coocood Nov 18, 2019

tiancaiamao Nov 18, 2019

coocood Nov 18, 2019

tiancaiamao Nov 18, 2019

tiancaiamao commented Nov 18, 2019

coocood Nov 18, 2019

tiancaiamao Nov 18, 2019 •

edited

Loading

MyonKeminta Nov 18, 2019

tiancaiamao Nov 18, 2019

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

MyonKeminta left a comment

youjiali1995 left a comment

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

coocood left a comment

store: update kvproto.CheckTxnStatus response #13432

store: update kvproto.CheckTxnStatus response #13432

Conversation

tiancaiamao commented Nov 13, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

tiancaiamao commented Nov 13, 2019

tiancaiamao commented Nov 13, 2019

codecov bot commented Nov 13, 2019 • edited Loading

Codecov Report

tiancaiamao commented Nov 13, 2019

tiancaiamao commented Nov 15, 2019

youjiali1995 left a comment

Choose a reason for hiding this comment

coocood commented Nov 15, 2019

coocood commented Nov 15, 2019

sre-bot commented Nov 15, 2019

sre-bot commented Nov 15, 2019

tiancaiamao commented Nov 15, 2019

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

coocood Nov 18, 2019

Choose a reason for hiding this comment

tiancaiamao Nov 18, 2019

Choose a reason for hiding this comment

coocood Nov 18, 2019

Choose a reason for hiding this comment

tiancaiamao Nov 18, 2019

Choose a reason for hiding this comment

tiancaiamao commented Nov 18, 2019

coocood Nov 18, 2019

Choose a reason for hiding this comment

tiancaiamao Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

MyonKeminta Nov 18, 2019

Choose a reason for hiding this comment

tiancaiamao Nov 18, 2019

Choose a reason for hiding this comment

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

MyonKeminta left a comment

Choose a reason for hiding this comment

youjiali1995 left a comment

Choose a reason for hiding this comment

tiancaiamao commented Nov 18, 2019

tiancaiamao commented Nov 18, 2019

coocood left a comment

Choose a reason for hiding this comment

tiancaiamao commented Nov 13, 2019 •

edited

Loading

codecov bot commented Nov 13, 2019 •

edited

Loading

tiancaiamao Nov 18, 2019 •

edited

Loading