ddl: integrate fast create table into normal general DDL workflow #55025
Conversation
ti-chi-bot
bot
added
do-not-merge/work-in-progress
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #55025 +/- ##
================================================
+ Coverage 72.8418% 75.4601% +2.6183%
================================================
Files 1560 1566 +6
Lines 438616 448047 +9431
================================================
+ Hits 319496 338097 +18601
+ Misses 99538 89269 -10269
- Partials 19582 20681 +1099
Flags with carried forward coverage won't be shown. Click here to find out more.
|
D3Hunter
changed the title
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
| e.limitJobCh <- task | ||
| } | ||
| // TODO this might block forever, as the consumer part considers context cancel. | ||
| e.limitJobCh <- task | ||
| } | ||
|
|
||
| // addBatchDDLJobsV1 gets global job IDs and puts the DDL jobs in the DDL queue. | ||
| func (d *ddl) addBatchDDLJobsV1(jobWs []*JobWrapper) { |
There was a problem hiding this comment.
maybe we can revert the name to addBatchDDLJobs in later pr.
| @@ -10139,19 +10154,20 @@ func (d *ddl) addBatchDDLJobsV1(jobWs []*JobWrapper) { | |||
| logutil.DDLLogger().Info("add DDL jobs", | |||
| zap.Int("batch count", len(jobWs)), | |||
| zap.String("jobs", jobs), | |||
| zap.Bool("table", toTable)) | |||
| zap.Bool("table", toTable), | |||
| zap.Bool("fast_create", fastCreate)) | |||
| } | |||
| } | |||
|
|
|||
| // addBatchLocalDDLJobs gets global job IDs and delivery the DDL jobs to local TiDB | |||
| func (d *ddl) addBatchLocalDDLJobs(jobWs []*JobWrapper) { | |||
There was a problem hiding this comment.
will not be called currently?
There was a problem hiding this comment.
yes, some code are not removed currently, as this pr is quite large, will do it later
| } | ||
| jobs = append(jobs, jobW.Job) | ||
| // CreateTables only support tables of same schema now. |
There was a problem hiding this comment.
Why do we have this limitation?
There was a problem hiding this comment.
initial design has this limit #28763
we want to extend it to support tables of different schema, but requires CDC to support it too, maybe in 8.4 or later
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, tangenta, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: ref #54436
Problem Summary:
What changed and how does it work?
Compared with current general DDL, the speedup of fast-create-table mostly due to merging multiple CreateTable jobs into 1 CreateTables job, and running them together and disable wait schema version synchonization. So we have to use a larger concurrency to make them merge as we are merging from the job channel. Without merging jobs, such as without enough concurrency, it's even slower than current general DDL, as it has less workers in most cases. Normal general DDL can merge jobs too.
current fast-create-table is harder to support creating tables with foreign keys without wait schema version change, and without waiting for schema version synced, it causes schema inconsistency for half ddl lease which is expected. Normal general DDL don't have such issues.
Check List
Tests
same case in #54693, with fast-create flag enabled, takes 9m4. when run with 100 threads, takes 4m20
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.