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.

Sign up for GitHub

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

Jump to bottom

mockstore: introducing embedded unistore #17156

Merged
merged 3 commits into from
May 18, 2020
Merged

mockstore: introducing embedded unistore #17156

merged 3 commits into from
May 18, 2020

Conversation

bobotu
Copy link
Contributor

@bobotu bobotu commented May 13, 2020 •
edited
Loading

What problem does this PR solve?

Integrating unistore into TiDB, so we don't need to maintain a storage backend for testing.

The unistore backend has much better performance, so you can start a single node TiDB by set -store=unistore to check your performance optimizations.

What is changed and how it works?

The main work is done in ngaut/unistore#382. TiDB side just needs to wrap the tools provided by unistore to implement the test toolkit's interface.

In order to avoid the impact of the introduced bug on CI, I temporarily retained mocktikv and provided configuration to switch.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note

@bobotu bobotu requested review from lysu, coocood and tiancaiamao May 13, 2020 04:45
@ngaut
Copy link
Member

ngaut commented May 13, 2020

Good job, looking forward to seeing amazing performance improvements.

bobotu
bobotu commented May 13, 2020
View reviewed changes
server/http_handler_test.go Show resolved Hide resolved
store/mockstore/mockstore.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 13, 2020 •
edited
Loading

Codecov Report

Merging #17156 into master will decrease coverage by 0.0360%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #17156        +/-   ##
================================================
- Coverage   79.9908%   79.9547%   -0.0361%     
================================================
  Files           511        517         +6     
  Lines        139236     141091      +1855     
================================================
+ Hits         111376     112809      +1433     
- Misses        18895      19319       +424     
+ Partials       8965       8963         -2

@bobotu
Copy link
Contributor Author

bobotu commented May 13, 2020

/run-all-tests

ngaut
ngaut reviewed May 14, 2020
View reviewed changes
server/http_handler_test.go Outdated Show resolved Hide resolved
@bobotu bobotu force-pushed the unistore branch 2 times, most recently from 6512d84 to 546b34d Compare May 15, 2020 07:11
@bobotu bobotu marked this pull request as ready for review May 15, 2020 08:07
@bobotu bobotu requested review from a team as code owners May 15, 2020 08:07
@ghost ghost requested review from XuHuaiyu and eurekaka and removed request for a team May 15, 2020 08:07
@bobotu
Copy link
Contributor Author

bobotu commented May 15, 2020

/run-all-tests

@bobotu
Copy link
Contributor Author

bobotu commented May 15, 2020

The default mockstore for testing is still the mocktikv, I'll change it after some bug in unistore is stable enough to run CI cases.

@coocood
Copy link
Member

coocood commented May 15, 2020

LGTM

@ngaut
Copy link
Member

ngaut commented May 15, 2020

Could you file an issue to reflect the future plan?

The default mockstore for testing is still the mocktikv, I'll change it after some bug in unistore is stable enough to run CI cases.

lysu
lysu reviewed May 18, 2020
View reviewed changes
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.

rest LGTM

tidb-server/main.go
err = kvstore.Register("mocktikv", mockstore.MockDriver{})
err = kvstore.Register("mocktikv", mockstore.MockTiKVDriver{})
terror.MustNil(err)
err = kvstore.Register("unistore", mockstore.EmbedUnistoreDriver{})
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we also need to add "unistore" to main.go:119, https://github.com/pingcap/tidb/pull/17156/files#diff-af2c9279ed41f7023f9b6b73cba2d18fR119

@lysu lysu added the status/LGT2 Indicates that a PR has LGTM 2. label May 18, 2020
@coocood
Copy link
Member

coocood commented May 18, 2020

/merge

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

sre-bot commented May 18, 2020

/run-all-tests

@sre-bot sre-bot merged commit ac30f53 into pingcap:master May 18, 2020
@bobotu bobotu mentioned this pull request May 18, 2020
Closed
3 tasks
@bobotu bobotu deleted the unistore branch May 18, 2020 09:31
@bobotu
Copy link
Contributor Author

bobotu commented May 18, 2020

Could you file an issue to reflect the future plan?

The default mockstore for testing is still the mocktikv, I'll change it after some bug in unistore is stable enough to run CI cases.

#17266

@MyonKeminta MyonKeminta mentioned this pull request Jun 2, 2020
Closed
@kolbe kolbe mentioned this pull request Mar 29, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lysu lysu lysu left review comments

@ngaut ngaut ngaut left review comments

@cfzjywxk cfzjywxk cfzjywxk left review comments

@coocood coocood coocood approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

@eurekaka eurekaka Awaiting requested review from eurekaka

Assignees
No one assigned
Labels
component/config component/expression component/test component/unistore sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bobotu @ngaut @coocood @sre-bot @lysu @cfzjywxk