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

import: Optimize importer overall processing speed #4245

Closed
wants to merge 9 commits into from
Closed

import: Optimize importer overall processing speed #4245

wants to merge 9 commits into from

Conversation

lonng
Copy link
Member

@lonng lonng commented Feb 20, 2019

What have you changed? (mandatory)

Refactor concurrency mode of ImportJob

Implement the preemptive concurrency mode.

  • Before

    parallel for each range of ranges {
        create sst channel
        create import threads
        parallel for each thread of import threads do {
            recv sst from sst channel
            upload sst
            ingest sst
        }
        loop {
            open SSTStreamFile
            for each sst from stream {
                send sst to sst channel
            }
        }
    }
    
  • After

    create dispatch range channel
    create dispatch thread do {
        for each range of ranges {
            send range to range channel
        }
    }
    create sst channel
    create split threads
    parallel for each thread of split threads do {
        recv range from range channel
        open SSTStreamFile
        for each sst from stream {
            send sst to sst channel
        }
    }
    create import threads
    parallel for each thread of import threads do {
        recv sst from sst channel
        upload sst
        ingest sst
    }
    
    

Memory optimization

  • Avoid reading the entire file into memory.
  • Use disk based Env instead of memory based.

Redesign retry range strategy

Retry entire range when any sst import failed.

Implement a speed limit for tikv-importer

Implemented a maximum speed limit for uploading from Importer to TiKV, using Token Bucket algorithm. The speed limit is needed to avoid saturating the network bandwidth which causes PD to assume TiKV nodes went down due to heartbeat not going through.

What are the type of the changes? (mandatory)

The currently defined types are listed below, please pick one of the types for this PR by removing the others:

  • New feature (change which adds functionality)
  • Improvement (change which is an improvement to an existing feature)
  • Bug fix (change which fixes an issue)
  • Misc (other changes)

How has this PR been tested? (mandatory)

Manual tested.

Does this PR affect documentation (docs) update? (mandatory)

No.

Does this PR affect tidb-ansible update? (mandatory)

No.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@lonng lonng changed the title import: Optimize importer overall processing speed [DNM]import: Optimize importer overall processing speed Feb 20, 2019
@Connor1996 Connor1996 added component/rocksdb Component: RocksDB engine S: DNM labels Feb 20, 2019
@lonng
Copy link
Member Author

lonng commented Feb 22, 2019

Related unexpected memory usage issue: #4257

@DorianZheng DorianZheng changed the title [DNM]import: Optimize importer overall processing speed import: Optimize importer overall processing speed Feb 25, 2019
@lonng
Copy link
Member Author

lonng commented Feb 26, 2019

@DorianZheng Please remove the S:DNM label, thanks.

@nolouch nolouch removed the S: DNM label Feb 27, 2019
kennytm and others added 8 commits March 7, 2019 22:19
This makes the SST size to upload larger, which reduces the number of SST
files needed to ingested, and improve overall speed.

Signed-off-by: kennytm <kennytm@gmail.com>
The bottle-neck of Importer is in SST ingestion on the TiKV side, not
disk I/O. Storing these SST in RAM will just increase the chance Importer
getting killed by OOM.

Signed-off-by: Lonng <chris@lonng.org>
Signed-off-by: kennytm <kennytm@gmail.com>
Jobs are now processed dynamically using a channel instead of predestined,
to prevent a single slow thread blocking the entire process.

Signed-off-by: Lonng <chris@lonng.org>
Implemented a maximum speed limit for uploading from Importer to TiKV,
using Token Bucket algorithm. The speed limit is needed to avoid
saturating the network bandwidth which causes PD to assume TiKV nodes
went down due to heartbeat not going through.

Signed-off-by: kennytm <kennytm@gmail.com>
Signed-off-by: Lonng <chris@lonng.org>
Signed-off-by: kennytm <kennytm@gmail.com>
@zhouqiang-cl
Copy link
Contributor

/test

@kennytm
Copy link
Contributor

kennytm commented Mar 8, 2019

@huachaohuang PTAL

@huachaohuang
Copy link
Contributor

This PR is a combination of multiple optimizations, we should split it into multiple single-purpose PRs.

@kennytm
Copy link
Contributor

kennytm commented Mar 11, 2019

@huachaohuang if we split this into 8 PRs, do we also file 8 additional cherrypick-to-2.1 PRs or a single one combining all 8?

@huachaohuang
Copy link
Contributor

@kennytm cherry picks can be combined into one PR.

@kennytm
Copy link
Contributor

kennytm commented Mar 11, 2019

@huachaohuang I've split this into 5 PRs (see the references above). There are 2 remaining PRs which rely on #4349, I'll file them after #4349 is merged.

@lonng
Copy link
Member Author

lonng commented May 18, 2019

All extracted PRs merged.

@lonng lonng closed this May 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/rocksdb Component: RocksDB engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants