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

restore: optimize SQL processing speed #110

Merged
merged 6 commits into from
Jan 2, 2019
Merged

restore: optimize SQL processing speed #110

merged 6 commits into from
Jan 2, 2019

Conversation

lonng
Copy link
Contributor

@lonng lonng commented Dec 25, 2018 •
edited
Loading

DNM: Based on #109 for simplicity of development.

What problem does this PR solve?

Optimizing SQL processing speed

Result:

  • PR 109: TableConcurrency = 20, RegionConcurrency = 40, the metrics data has been lost due to cluster be cleanup.

    Data size: 340G
Rate: ~45MB/s
Total time: ~= 2h30m (import time: 40m)

  • PR 110: TableConcurrency = 20, RegionConcurrency = 20, IOConcurrency = 5, Test1 metrics snapshot, Test2 metrics snapshot

    CAN NOT REPREDUCE [IO Delay unstable]

    Test1: 
Data size: 146G
Rate: 90~160MB/s
Total time: ~= 48m (import time: 27m)

Test2:
Data size: 146G
Rate: 130~190MB/s
Total time: ~= 46m (import time: 28m)

Test3
coming ...

  • PR 110: TableConcurrency = 40, RegionConcurrency = 40, IOConcurrency = 10 160G, Metrics

    2018/12/30 01:16:32.871 restore.go:477: [info] restore all tables data takes 52m59.59960385s
2018/12/30 01:16:32.871 restore.go:366: [info] Everything imported, stopping periodic actions
2018/12/30 01:16:32.871 restore.go:208: [error] run cause error : [types:1292]invalid time format: '{2038 1 19 4 4 36 0}'
2018/12/30 01:16:32.871 restore.go:214: [info] the whole procedure takes 53m7.986292573s

Early conclusion:

Concurrent IO causes delays to lengthen, which lengthens SQL processing time

What is changed and how it works?

Limiting IO concurrency

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

@lonng lonng added the status/DNM Do not merge, test is failing or blocked by another PR label Dec 25, 2018
@sre-bot
Copy link

sre-bot commented Dec 25, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@lonng
Copy link
Contributor Author

lonng commented Dec 25, 2018

/run-integration-test

@lonng lonng force-pushed the lonng/opt-sql-proc branch 2 times, most recently from 9249430 to 0bb02d6 Compare December 25, 2018 07:03
@lonng
Copy link
Contributor Author

lonng commented Dec 25, 2018

/run-integration-test

@lonng lonng force-pushed the lonng/opt-sql-proc branch from 459ceaf to 07232e3 Compare December 26, 2018 05:41
@lonng
Copy link
Contributor Author

lonng commented Dec 26, 2018

/run-integration-test

@lonng lonng changed the base branch from master to kennytm/1-chunk-1-file December 26, 2018 06:34
@lonng lonng changed the base branch from kennytm/1-chunk-1-file to master December 26, 2018 06:35
@lonng lonng added the Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated label Dec 26, 2018
@lonng lonng force-pushed the lonng/opt-sql-proc branch from 2a3e7f9 to e96ca9c Compare December 26, 2018 07:12
@lonng
Copy link
Contributor Author

lonng commented Dec 26, 2018

/run-integration-test

@lonng lonng force-pushed the lonng/opt-sql-proc branch 2 times, most recently from c7f283b to 7315ef5 Compare December 26, 2018 08:58
@IANTHEREAL
Copy link
Collaborator

IANTHEREAL commented Dec 26, 2018 •
edited
Loading

add test result into description, like io conc = 1, default, region conc...

@lonng lonng force-pushed the lonng/opt-sql-proc branch 2 times, most recently from ccfe5ef to 6baedce Compare December 27, 2018 00:34
@lonng lonng force-pushed the lonng/opt-sql-proc branch 3 times, most recently from 5fc836f to 79efc70 Compare December 30, 2018 03:59
@lonng
Copy link
Contributor Author

lonng commented Dec 30, 2018

/run-integration-test

@lonng lonng added status/PTAL This PR is ready for review. Add this label back after committing new changes priority/normal type/feature New feature and removed status/DNM Do not merge, test is failing or blocked by another PR labels Dec 30, 2018
@lonng
Copy link
Contributor Author

lonng commented Dec 30, 2018

@GregoryIan @kennytm @july2993 PTAL

@kennytm
Copy link
Collaborator

kennytm commented Dec 30, 2018

/run-unit-test

kennytm
kennytm reviewed Dec 30, 2018
View reviewed changes
go.mod Outdated Show resolved Hide resolved
lightning/metric/metric.go Outdated Show resolved Hide resolved
lightning/metric/metric.go Outdated Show resolved Hide resolved
lightning/mydump/parser.go Outdated Show resolved Hide resolved
lightning/mydump/parser.go Outdated Show resolved Hide resolved
tidb-lightning.toml Outdated Show resolved Hide resolved
@lonng lonng force-pushed the lonng/opt-sql-proc branch 2 times, most recently from c15fa92 to 9139e2e Compare December 30, 2018 07:55
@lonng
Copy link
Contributor Author

lonng commented Dec 30, 2018

/run-all-tests

kennytm
kennytm reviewed Dec 30, 2018
View reviewed changes
Copy link
Collaborator

@kennytm kennytm 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/config/config.go Outdated Show resolved Hide resolved
@lonng lonng force-pushed the lonng/opt-sql-proc branch from 9139e2e to fef33c2 Compare December 30, 2018 08:28
@lonng
Copy link
Contributor Author

lonng commented Dec 30, 2018

/run-all-tests

kennytm
kennytm reviewed Dec 30, 2018
View reviewed changes
tidb-lightning.toml Outdated Show resolved Hide resolved
@kennytm kennytm 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 Dec 30, 2018
@lonng lonng force-pushed the lonng/opt-sql-proc branch from fef33c2 to 54688f3 Compare December 30, 2018 08:54
@IANTHEREAL
Copy link
Collaborator

Rest LGTM

@lonng
Copy link
Contributor Author

lonng commented Jan 2, 2019

/run-all-tests

@IANTHEREAL
Copy link
Collaborator

some problems with ci

@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 Jan 2, 2019
@IANTHEREAL
Copy link
Collaborator

well done! LGTM

@kennytm
Copy link
Collaborator

kennytm commented Jan 2, 2019

/run-integration-test

@kennytm kennytm merged commit fdf4d5b into master Jan 2, 2019
@kennytm kennytm deleted the lonng/opt-sql-proc branch January 2, 2019 13:52
@kennytm kennytm added Should Update Ansible The config in TiDB-Ansible should be updated and removed Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Jan 8, 2019
@kennytm kennytm removed the Should Update Ansible The config in TiDB-Ansible should be updated label May 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IANTHEREAL IANTHEREAL IANTHEREAL left review comments

@kennytm kennytm kennytm left review comments

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.
4 participants
@lonng @sre-bot @IANTHEREAL @kennytm