-
Notifications
You must be signed in to change notification settings - Fork 101
restore: split & scatter regions concurrently #1363
Conversation
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/build |
pkg/restore/pipeline_items.go
Outdated
files := r.result.Files() | ||
// There has been a worker in the `RestoreFiles` procedure. | ||
// Spawning a raw goroutine won't make too many requests to TiKV. | ||
go b.client.RestoreFilesAndThen(ctx, files, r.result.RewriteRules, b.updateCh, func(e error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about use errgroup.WithContext
pkg/restore/util.go
Outdated
updateCh glue.Progress, | ||
) error { | ||
ch := make(chan error) | ||
go SplitRangesAndThen(ctx, client, ranges, rewriteRules, updateCh, func(err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
var ret error
SplitRangesAndThen(ctx, client, ranges, rewriteRules, updateCh, func(err error) {
ret = err
})
return ret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even I don't know why, this ruins the whole procedure.
Thankfully, after using errgroup
, we don't need this change anymore. 😄
} | ||
wg.(*sync.WaitGroup).Wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a single global WaitGroup
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We make a waitGroup
for each tables, so once some table has been restored fully (i.e. the wait group for it was done), we can send it to checksumming immediately.
closing this as it has been moved to pingcap/tidb#27034. |
What problem does this PR solve?
Before, when restoring many small tables, the batcher would probably send small batch due to the so called
AutoCommit
feature of the batcher. By this, we can make the split & scatter & restore worker more active.But frequently send small batches isn't free. The split step is costly and I/O bounded for even small batches. For example, it costs about 3s to splitting 60 ranges, but restore those ranges typically costs only 1s. Then the restore worker get idle at most time. The restore hence has slowed down.
What is changed and how it works?
Instead of using a single split worker, this PR allow multi restore batches be split concurrently.
We added two hidden flags,
--batch-flush-interval
and--pd-concurrency
, the former for better tuning the behavior of batcher, the latter for tweaking the concurrent split.Also, more logs were added so the create table speed, download, ingest time cost can be observed via log.
Check List
Tests
A internal test shows, in a 190GB, 6000 tables workload, this PR can speed up the restoration: the original version takes over 2 hours for restoring, and this version takes about 30mins for restoring. The latter is nearly equal to the time cost of creating tables(see figure below).
Release note