ddl: initial support for parallel DDL by zimulala · Pull Request #6955 · pingcap/tidb

zimulala · 2018-07-02T07:42:23Z

What have you changed? (mandatory)

ddl: remove cleanAddIndexQueueJobs and initial support for parallel DDL.
Initial support for parallel DDL is as follows:

The DDL of "add index" and the other types of DDL can be executed parallelly when they are on the different tables. We use two queues to save the "add index" and other DDLs in storage. And we have two workers to handle these DDL jobs. The "add index" worker handles the "add index" queue. Another worker handles another queue.
If the DDL of "add index" and the other types of DDL are on the same table, we need to perform these two operations serially.

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:

Improvement

How has this PR been tested (mandatory)?

unit test

Does this PR affect documentation (docs/docs-cn) update? (optional)

Yes.

crazycs520 · 2018-07-03T03:51:13Z

util/admin/admin.go

+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return append(generalJobs, addIdxJobs...), nil


The return jobs may be not sorted by job ID, should we return sorted jobs? Because the older function naturally return a sorted jobs

I think it can be done in the next PR. I added a "TODO" now.

shenli · 2018-07-03T09:52:56Z

ddl/ddl.go

 			// TODO: Add the type of DDL worker.
 			metrics.DDLCounter.WithLabelValues(metrics.CreateDDLWorker).Inc()
+
+			// For every start, we will send a fake job to let worker


What is For every start?

It's original comment. I think it means "For each call to the start function".

shenli · 2018-07-03T09:53:26Z

ddl/ddl.go

 }

+func (d *ddl) asyncNotifyWorker(jobTp model.ActionType) {
+	// If the workes don't run, we needn't to notice workers.


workers or works?

notify is better than notice.

ciscoxll · 2018-07-06T08:52:43Z

ddl/ddl_worker.go

 	return errors.Trace(err)
 }

+func isDependencyJobDone(t *meta.Meta, job *model.Job) (bool, error) {


Will there be multiple job DDL dependencies?

We only record the maximum job ID in multiple dependent jobs.

zimulala · 2018-07-07T05:42:07Z

/run-all-tests

zimulala · 2018-07-09T06:37:56Z

/run-all-tests

zimulala · 2018-07-09T08:52:51Z

/run-common-test
/run-integration-common-test

zimulala · 2018-07-09T10:53:00Z

/run-common-test

zimulala · 2018-07-11T12:04:54Z

PTAL @shenli @ciscoxll @crazycs520 @tiancaiamao

shenli · 2018-07-11T16:03:55Z

ddl/ddl.go

s/notice/notify/

shenli · 2018-07-11T16:05:17Z

ddl/ddl_worker.go

What is this used for?

ddlJobCh used to be member of ddl, but now, we have 2 kinds of workers, we need two ddlJobCh for every kind of worker. So move ddlJobCh from ddl to worker

shenli · 2018-07-11T16:08:58Z

util/admin/admin.go

Do not need to mention the error.

Other functions also mention the error.

This comment provides nothing more than the function name.

ciscoxll

reset

misoperation

tiancaiamao · 2018-07-13T02:30:38Z

ddl/ddl.go

Worker id is always 0 ?

tiancaiamao · 2018-07-13T02:44:51Z

ddl/ddl_worker.go

Why it need to be make(chan struct{}, 1 ) rather than make(chan struct{})

We need to push info to the channel.

tiancaiamao · 2018-07-13T02:50:33Z

ddl/ddl_worker.go

%s to print time? what's the result looks like

why not print worker type?

It's the old code, I will handle it.

tiancaiamao · 2018-07-13T03:19:57Z

ddl/ddl_worker.go

It's hard to understand here.

It's also hard to understand for me.

Please add comments.

I don't think add comment helps @shenli
meta.Meta should not store the information about queue key.
If the caller has to modify status in meta.Meta, before calling its method, why not provide that status as argument?

tiancaiamao · 2018-07-13T05:13:43Z

ddl/ddl_worker.go

I suggest rename GetAllDDLJobs to GetDDLJobsInQueue and pass the queue, rather than change meta's jobListKey status, it's very tricky.

GetAllDDLJobs definitely get jobs from mDDLJobListKey? Is that correct?

mDDLJobListKey will change between two queue, I don't know which one, that's the problem.

So I suggest:

GetDDLJobsInQueue(general) GetDDLJobsInQueue(addindex)

Please address comment

Done, I changed the name of the function.

tiancaiamao · 2018-07-13T05:18:02Z

ddl/ddl_worker.go

Will t.GetHistoryDDLJob select the right job queue?

Yes, it set key at https://github.com/pingcap/tidb/pull/6955/files#diff-625e9ab73a4a85a4a64f659183d95f3dR336.

It is better to find the job in the waiting job list. Because the history job list maybe long.

tiancaiamao · 2018-07-13T05:40:16Z

ddl/ddl_worker.go

There will be read/write conflict in GetALLDDLJobs, because each worker check the other worker's queue to dependence.
Is the error retryable and properly handled?

It will be retried.

tiancaiamao · 2018-07-13T05:44:21Z

ddl/ddl_worker_test.go

s/for use by other tests/for other tests

tiancaiamao · 2018-07-13T05:48:00Z

ddl/ddl_worker_test.go

I find a strange char here

tiancaiamao · 2018-07-13T05:52:39Z

util/admin/admin.go

Add an else branch so that the code is more robust without the assumption about job.Type default value.

shenli · 2018-07-16T01:51:38Z

ddl/ddl.go

s/Notice/Notify/g

shenli · 2018-07-16T01:52:21Z

ddl/ddl_test.go

buildRebaseAutoIDJob

shenli · 2018-07-16T01:54:35Z

ddl/ddl_worker.go

Please add comments.

shenli · 2018-07-16T01:55:25Z

ddl/ddl_worker.go

How about "current DDL job %v depends on job %v"?

shenli · 2018-07-16T01:58:23Z

ddl/ddl_worker.go

If waitDependencyJob is true, it is not an error.

Yes, but we'd better wait a moment. I will add a comment for it.

shenli · 2018-07-16T02:00:51Z

/run-all-tests

winkyao · 2018-07-18T10:05:55Z

ddl/ddl.go

 		store:        store,
 		lease:        lease,
-		ddlJobCh:     make(chan struct{}, 1),
 		ddlJobDoneCh: make(chan struct{}, 1),


Could we move the ddlJobDoneCh to the different worker?

winkyao · 2018-07-18T10:25:31Z

ddl/ddl_worker.go

+	if historyJob == nil {
+		return false, nil
+	}
+	log.Infof("[ddl] DDL job %v isn't dependent on job ID %d", job, job.DependencyID)


DDL job %v isn't dependent on job ID %d ? What about DDL job %v dependent job ID %d is finished ?

winkyao · 2018-07-18T10:30:25Z

util/admin/admin.go

 			}
-			err = t.UpdateDDLJob(int64(j), job, true)
+			if job.Type == model.ActionAddIndex {
+				err = t.UpdateDDLJob(int64(j), job, true, meta.AddIndexJobListKey)


Why not just new a meta with meta.AddIndexJobListKey?

We have a meta here, I think it's OK.

I mean we can use a new meta to avoid add a param to the function.

shenli · 2018-07-20T03:25:12Z

ddl/ddl.go

@@ -413,11 +411,9 @@ func (d *ddl) genGlobalID() (int64, error) {

 // generalWorker returns the first worker. The ddl structure has only one worker before we implement the parallel worker.


Need to update the comment.

zimulala · 2018-07-23T11:34:00Z

PTAL @winkyao @shenli

shenli · 2018-07-23T14:27:39Z

@zimulala Please resolve the conflicts.

shenli · 2018-07-23T14:57:01Z

ddl/ddl_worker.go

 }

+func isDependencyJobDone(t *meta.Meta, job *model.Job) (bool, error) {
+	if job.DependencyID == 0 {


Please create a constant for 0 or add comment for the if statement.

shenli · 2018-07-23T14:58:47Z

ddl/ddl_worker.go

+	return true, nil
+}
+
+func newMetaWithQueueTp(txn kv.Transaction, tp string) *meta.Meta {


Please address it.

shenli · 2018-07-23T15:04:51Z

meta/meta.go

 // NewMeta creates a Meta in transaction txn.
-func NewMeta(txn kv.Transaction) *Meta {
+// If the current Meta needs to handle a job, jobListKey is the type of the job's list.
+// We don't change the value of the jobListKey in a Meta.


Can not understand the comment.

shenli · 2018-07-24T02:55:02Z

ddl/db_change_test.go

 			kv.RunInNewTxn(s.store, false, func(txn kv.Transaction) error {
 				m := meta.NewMeta(txn)
+				// Get the number of jobs from the adding index queue.
+				addIdxLen, err1 := m.DDLJobQueueLen(meta.AddIndexJobListKey)


Can we use GetDDLJobs and get the length of the return value?

shenli · 2018-07-24T03:03:16Z

ddl/ddl_worker.go

 	job.Query, _ = ctx.Value(sessionctx.QueryString).(string)
 	err := kv.RunInNewTxn(d.store, true, func(txn kv.Transaction) error {
-		t := meta.NewMeta(txn)
+		t := newMetaWithQueueTp(txn, job.Type.String())


Why not use meta.AddIndexJobListKey as the second parameter?

The job may not be add index, So has to according to the job type to create meta?

Almost. I want to put the check of job type or worker type into newMetaWithQueueTp.

shenli · 2018-07-24T03:08:14Z

ddl/ddl_worker.go

 			// No job now, return and retry getting later.
 			return nil
 		}
+		w.waitDependencyJobFinished(job, &waitDependencyJobCnt)


If its dependencyJob is not done yet, it would return at line 357. So why we need to wait here?

line 357 return is in a txn func, not return in handleDDLJobQueue func

As @crazycs520 said. And if put it in line357, we need wait for 200ms. I am afraid this txn is easy to conflict. So I put it here.

shenli · 2018-07-24T03:12:08Z

meta/meta.go

 // NewMeta creates a Meta in transaction txn.
-func NewMeta(txn kv.Transaction) *Meta {
+// If the current Meta needs to handle a job, jobListKey is the type of the job's list.
+func NewMeta(txn kv.Transaction, jobListKeys ...JobListKeyType) *Meta {


Can we always specify the JobListKey?

A lot of places use this function, so I use this method to handle it.
And In other packages, I think we needn't distinguish the type of jobListKeys.

shenli

LGTM

winkyao

LGTM

zimulala · 2018-07-24T13:37:00Z

/run-all-tests

zimulala · 2018-07-25T02:17:23Z

/run-common-test
/run-integration-ddl-test

zimulala · 2018-07-25T02:28:52Z

/run-common-test -tidb-test=pr/592
/run-integration-common-test -tidb-test=pr/592

zimulala added status/DNM labels Jul 2, 2018

shenli requested a review from winkyao July 2, 2018 07:54

zimulala force-pushed the parallel-ddl branch from 0b64181 to f57c901 Compare July 2, 2018 07:58

zimulala added 2 commits July 2, 2018 18:46

ddl: initial support for parallel DDL

ee4ad7c

Merge branch 'master' into zimuxia/parallel-ddl

7d35817

zimulala force-pushed the parallel-ddl branch from f57c901 to 7d35817 Compare July 2, 2018 10:54

crazycs520 reviewed Jul 3, 2018

View reviewed changes

shenli reviewed Jul 5, 2018

View reviewed changes

ddl, util: address comments

065358e

ciscoxll reviewed Jul 6, 2018

View reviewed changes

zimulala removed the status/DNM label Jul 9, 2018

zimulala added the status/all tests passed label Jul 9, 2018

shenli reviewed Jul 11, 2018

View reviewed changes

crazycs520 previously approved these changes Jul 12, 2018

View reviewed changes

shenli self-assigned this Jul 12, 2018

ciscoxll reviewed Jul 12, 2018

View reviewed changes

tiancaiamao reviewed Jul 13, 2018

View reviewed changes

shenli reviewed Jul 16, 2018

View reviewed changes

zimulala added 4 commits July 16, 2018 14:49

*: address comments

7f8112b

ddl: update test

eb969f5

Merge branch 'master' into zimuxia/parallel-ddl

3c09d09

ddl: make test happy

3e4503a

winkyao reviewed Jul 18, 2018

View reviewed changes

shenli reviewed Jul 20, 2018

View reviewed changes

zimulala added 3 commits July 23, 2018 15:00

util, ddl: address comments

b2116ec

ddl: address a comment

847ba5f

Merge branch 'master' into zimuxia/parallel-ddl

49251bc

shenli reviewed Jul 23, 2018

View reviewed changes

zimulala added 2 commits July 23, 2018 23:30

Merge branch 'master' into zimuxia/parallel-ddl

e7ba3e8

ddl, meta: address comments

f763802

shenli reviewed Jul 24, 2018

View reviewed changes

zimulala added 2 commits July 24, 2018 16:01

ddl: address a comment

734522f

store/mockstore: make golint happy

118bfb9

shenli approved these changes Jul 24, 2018

View reviewed changes

shenli added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 24, 2018

winkyao approved these changes Jul 24, 2018

View reviewed changes

Merge branch 'master' into parallel-ddl

0453a0a

Merge branch 'master' into parallel-ddl

d34e942

zimulala merged commit da6f0c1 into pingcap:master Jul 25, 2018

zimulala deleted the parallel-ddl branch July 25, 2018 02:48

This was referenced Aug 6, 2018

Support showing all currently running jobs when executing "admin show ddl" #7274

Merged

ddl: adjust the way of checking all schema versions #7319

Merged

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

lonng mentioned this pull request Jul 7, 2020

Support Concurrent DDL Execution #18396

Closed

3 tasks

		@@ -413,11 +411,9 @@ func (d *ddl) genGlobalID() (int64, error) {

		// generalWorker returns the first worker. The ddl structure has only one worker before we implement the parallel worker.

Conversation

zimulala commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

Does this PR affect documentation (docs/docs-cn) update? (optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimulala Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimulala commented Jul 7, 2018

Uh oh!

zimulala commented Jul 9, 2018

Uh oh!

zimulala commented Jul 9, 2018

Uh oh!

zimulala commented Jul 9, 2018

Uh oh!

zimulala commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciscoxll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zimulala commented Jul 2, 2018 •

edited

Loading

zimulala Jul 5, 2018 •

edited

Loading