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

store: implement non-block reading for Get and BatchGet under the large transaction protocol #13599

Merged
merged 8 commits into from Nov 27, 2019

Conversation

@tiancaiamao
Copy link
Contributor

tiancaiamao commented Nov 19, 2019

What problem does this PR solve?

Large transactions should not block the read operation.

We've handle the coprocessor reading in #11986, and this commit continues with snapshot reading.

What is changed and how it works?

Implement non-block read for snapshot.Get() and snapshot.BatchGet() function.

When those operations meet lock, ResolveLock and retry.
The SendReqCtx operation would not block in the retry as we pass the resolvedLocks.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
…ge transaction protocol
@tiancaiamao tiancaiamao requested a review from coocood Nov 19, 2019
@tiancaiamao

This comment has been minimized.

Copy link
Contributor Author

tiancaiamao commented Nov 19, 2019

@tiancaiamao

This comment has been minimized.

Copy link
Contributor Author

tiancaiamao commented Nov 19, 2019

/run-all-tests

@tiancaiamao

This comment has been minimized.

Copy link
Contributor Author

tiancaiamao commented Nov 19, 2019

Integration test fails, we also need to implement this in TiKV? @MyonKeminta @youjiali1995

[2019-11-19T17:06:20.975Z] ----------------------------------------------------------------------
[2019-11-19T17:06:20.975Z] FAIL: snapshot_test.go:211: testSnapshotSuite.TestSkipLargeTxnLock
[2019-11-19T17:06:20.975Z] 
[2019-11-19T17:06:20.975Z] snapshot_test.go:232:
[2019-11-19T17:06:20.975Z]     // Commit txn, check the final commit ts is pushed.
[2019-11-19T17:06:20.975Z]     c.Assert(committer.commitKeys(bo, committer.keys), IsNil)
[2019-11-19T17:06:20.975Z] ... value *errors.fundamental = tikv aborts txn: Txn(InvalidTxnTso { start_ts: 412662674766430256, commit_ts: 0 }) ("tikv aborts txn: Txn(InvalidTxnTso { start_ts: 412662674766430256, commit_ts: 0 })")
[2019-11-19T17:06:20.975Z] 
[2019-11-19T17:06:20.975Z] [2019/11/20 01:06:17.109 +08:00] [INFO] [lock_resolver.go:540] ["resolveLock rollback"] [lock="key: []byte{0x78}, primary: []byte{0x78}, txnStartTS: 412662674766430256, ttl: 18, type: Put"]
[2019-11-19T17:06:20.975Z] [2019/11/20 01:06:17.110 +08:00] [INFO] [lock_resolver.go:540] ["resolveLock rollback"] [lock="key: []byte{0x79}, primary: []byte{0x78}, txnStartTS: 412662674766430256, ttl: 18, type: Put"]
[2019-11-19T17:06:20.975Z] 
[2019-11-19T17:06:20.975Z] ----------------------------------------------------------------------
tk.MustQuery("select * from t where id in (1)").Check(testkit.Rows("1 1"))

// Cover PointGet.
tk.MustQuery("select * from t where id = 1").Check(testkit.Rows("1 1"))

This comment has been minimized.

Copy link
@coocood

coocood Nov 20, 2019

Member

Does this query use MaxTS?
Then we need also to cover PointGet with real TS.

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 20, 2019

Author Contributor

Done

@tiancaiamao

This comment has been minimized.

Copy link
Contributor Author

tiancaiamao commented Nov 20, 2019

/integration-common-test

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 20, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13599   +/-   ##
===========================================
  Coverage   80.2676%   80.2676%           
===========================================
  Files           475        475           
  Lines        117898     117898           
===========================================
  Hits          94634      94634           
  Misses        15846      15846           
  Partials       7418       7418
@sre-bot sre-bot closed this Nov 20, 2019
@tiancaiamao tiancaiamao reopened this Nov 20, 2019
tiancaiamao added 2 commits Nov 20, 2019
@coocood

This comment has been minimized.

Copy link
Member

coocood commented Nov 20, 2019

LGTM

Copy link
Member

lysu left a comment

LGTM

@lysu lysu added the status/LGT2 label Nov 20, 2019
@@ -285,10 +298,17 @@ func (s *tikvSnapshot) get(bo *Backoffer, k kv.Key) ([]byte, error) {
}

failpoint.Inject("snapshot-get-cache-fail", func(_ failpoint.Value) {
panic("cache miss")
if bo.ctx.Value("TestSnapshotCache") != nil {

This comment has been minimized.

Copy link
@lysu

lysu Nov 20, 2019

Member

maybe using exp like 1*return(true)->return(false) like this

c.Assert(failpoint.Enable("github.com/pingcap/tidb/session/keepHistory", `1*return(true)->return(false)`), IsNil)
and check failpoint.Value == true at here is more gofail idiom~

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 20, 2019

Author Contributor

In the concurrent test environment, that mock panic may affect other tests.
bo.ctx.Value("TestSnapshotCache") is used to address that problem. @lysu

This comment has been minimized.

Copy link
@coocood

coocood Nov 20, 2019

Member

Or we can pass a TS and panic on equal TS.

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 21, 2019

Author Contributor

In failpoint.Inject it doesn't know which TS @coocood

@lysu
lysu approved these changes Nov 20, 2019
@coocood

This comment has been minimized.

Copy link
Member

coocood commented Nov 20, 2019

@coocood coocood requested a review from youjiali1995 Nov 27, 2019
Copy link
Contributor

youjiali1995 left a comment

LGTM

@coocood

This comment has been minimized.

Copy link
Member

coocood commented Nov 27, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link

sre-bot commented Nov 27, 2019

/run-all-tests

@sre-bot sre-bot merged commit c10c576 into pingcap:master Nov 27, 2019
14 checks passed
14 checks passed
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-copr-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
SunRunAway added a commit to SunRunAway/tidb that referenced this pull request Dec 2, 2019
edytagarbarz added a commit to edytagarbarz/tidb that referenced this pull request Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.