Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

*: support MySQL backend #221

Merged
merged 5 commits into from
Aug 14, 2019
Merged

*: support MySQL backend #221

merged 5 commits into from
Aug 14, 2019

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Jul 30, 2019

What problem does this PR solve?

Apply simplest change to support the MySQL backend.

What is changed and how it works?

  • Added the [tikv-importer] backend task configuration and --backend command line option.
  • When backend is "mysql", choose the new MySQL backend (when backend is "importer" choose the normal Importer backend)
  • The MySQL backend reuses the TiDB SQL connection. Therefore, we include the SQL mode when opening the connection.
  • The MySQL backend simply reconstructs a multi-valued INSERT statement. Prepared statements (INSERT VALUES (?,?,?)) is not used due to complication with hex/binary literals.

Caveats:

  • I have only enabled the MySQL backend on two integration tests for now. In the next PR I'll modify the integration test procedure so every test case will be tested against both Import and MySQL backends.
  • The current implementation is not tested for speed yet and may be slower than Loader.

Check List

Tests

  • Unit test
  • Integration test

Side effects

Related changes

  • Need to update the documentation
  • Need to update the tidb-ansible repository
  • Need to be included in the release note

@kennytm kennytm added Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated status/PTAL This PR is ready for review. Add this label back after committing new changes priority/normal type/feature New feature Should Update Ansible The config in TiDB-Ansible should be updated labels Jul 30, 2019
@kennytm kennytm force-pushed the kennytm/mysql-backend branch from e4d380f to 8341f95 Compare July 31, 2019 00:02
@kennytm
Copy link
Collaborator Author

kennytm commented Jul 31, 2019

/run-all-tests

csuzhangxc
csuzhangxc reviewed Aug 6, 2019
View reviewed changes
lightning/config/config.go Outdated Show resolved Hide resolved
lightning/config/config.go Outdated Show resolved Hide resolved
@IANTHEREAL IANTHEREAL requested a review from lance6716 August 8, 2019 07:46
lance6716
lance6716 reviewed Aug 8, 2019
View reviewed changes
Copy link
Contributor

@lance6716 lance6716 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-lightning.toml Outdated Show resolved Hide resolved
lightning/restore/tidb.go Show resolved Hide resolved
mock/backend.go Show resolved Hide resolved
@IANTHEREAL
Copy link
Collaborator

Good Job!

IANTHEREAL
IANTHEREAL reviewed Aug 13, 2019
View reviewed changes
lightning/kv/mysql.go Outdated Show resolved Hide resolved
IANTHEREAL
IANTHEREAL reviewed Aug 13, 2019
View reviewed changes
lightning/kv/mysql.go Outdated Show resolved Hide resolved
IANTHEREAL
IANTHEREAL reviewed Aug 13, 2019
View reviewed changes
lightning/kv/mysql.go Outdated Show resolved Hide resolved
@IANTHEREAL
Copy link
Collaborator

Rest LGTM

@IANTHEREAL
Copy link
Collaborator

@kennytm we can merge it ASAP after resolving these comments,

@lance6716
Copy link
Contributor

LGTM

@lance6716 lance6716 added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Aug 13, 2019
@kennytm
Copy link
Collaborator Author

kennytm commented Aug 13, 2019

/run-all-tests

(BTW zhouqiang tested this yesterday and found that the speed is 3x slower than Loader. The next step is to use pprof and find out why it's so slow)

@IANTHEREAL
Copy link
Collaborator

IANTHEREAL commented Aug 14, 2019 •
edited
Loading

agree with you @kennytm (but @zhouqiang-cl tell me that the speed is 10x slower than Loader) 😢

@kennytm
Copy link
Collaborator Author

kennytm commented Aug 14, 2019

/run-all-tests

19:44:53  GO111MODULE=off go get github.com/mattn/goveralls
19:46:15  Bad response status from coveralls: 405
19:46:15  <html>
19:46:15  <head><title>405 Not Allowed</title></head>
19:46:15  <body bgcolor="white">
19:46:15  <center><h1>405 Not Allowed</h1></center>
19:46:15  <hr><center>nginx</center>
19:46:15  </body>
19:46:15  </html>

@lance6716
Copy link
Contributor

seems coveralls is under maintenance

@kennytm
Copy link
Collaborator Author

kennytm commented Aug 14, 2019

😢

IANTHEREAL
IANTHEREAL approved these changes Aug 14, 2019
View reviewed changes
Copy link
Collaborator

@IANTHEREAL IANTHEREAL left a comment

Choose a reason for hiding this comment

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

LGTM

@IANTHEREAL IANTHEREAL added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Aug 14, 2019
@kennytm
Copy link
Collaborator Author

kennytm commented Aug 14, 2019

/run-all-tests

Update - Database is back online, processing queue is working through backlog.

Aug 13, 21:22 PDT

@kennytm kennytm merged commit 4eb74ec into master Aug 14, 2019
@kennytm kennytm deleted the kennytm/mysql-backend branch August 14, 2019 22:40
@kennytm kennytm removed Should Update Ansible The config in TiDB-Ansible should be updated Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Oct 30, 2019
@kennytm kennytm mentioned this pull request Oct 30, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ericsyh ericsyh ericsyh left review comments

@csuzhangxc csuzhangxc csuzhangxc left review comments

@lance6716 lance6716 lance6716 left review comments

@IANTHEREAL IANTHEREAL IANTHEREAL approved these changes

Assignees
No one assigned
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kennytm @IANTHEREAL @lance6716 @csuzhangxc @ericsyh