ddl, meta: clean the jobs in adding index queue #6161

zimulala · 2018-03-27T11:47:53Z

We will initially support the parallel DDL. So we will introduce the adding index queue.
This PR deals with the compatibility when the user chooses to downgrade.

zhexuany · 2018-03-28T02:12:05Z

ddl/ddl_worker.go

+ job.State = model.JobStateRollingback
+ job.Args = []interface{}{indexInfo.Name}
+ // If add index job rollbacks, its work is the same as drop index job do.
+ // When it's not in None state, the next state can be delete only state.


Maybe this way is more friendly for reader? // When it's not in "None" state, the next state can be "delete only" state.

zimulala · 2018-03-28T02:53:25Z

PTAL @winkyao @coocood @zhexuany

zimulala · 2018-03-28T02:59:39Z

/run-all-tests

zimulala · 2018-03-29T12:50:44Z

/run-all-tests

zhexuany · 2018-03-29T14:26:14Z

Please fix CI @zimulala

zimulala · 2018-03-30T02:28:33Z

/run-integration-common-test

winkyao · 2018-03-30T06:29:09Z

ddl/ddl_test.go

@@ -91,7 +91,6 @@ func checkHistoryJob(c *C, job *model.Job) {
 }

 func checkHistoryJobArgs(c *C, ctx sessionctx.Context, id int64, args *historyJobArgs) {
- c.Assert(ctx.NewTxn(), IsNil)


why remove?

Make TestCleanJobs stable.
I want to get schema version and check history job's schema version in a transaction.

winkyao · 2018-03-30T07:06:47Z

ddl/ddl_worker.go

+ return errors.Trace(err)
+ }
+ indexInfo := findIndexByName(indexName.L, tblInfo.Indices)
+ job.State = model.JobStateRollingback


why not use ddl.convert2RollbackJob ?

winkyao · 2018-03-30T07:09:49Z

ddl/ddl_worker.go

+ log.Infof("[ddl] cleaning job %v in the adding index queue.", job)
+
+ // The types of these jobs must be ActionAddIndex.
+ if job.SchemaState == model.StatePublic || job.SchemaState == model.StateNone {


Do we have to rollback or cancel the add index ddl job? What about enqueueing these jobs to default job list

@winkyao @coocood
If we put it in the default jobs list, we need to consider the order. And I hope this PR will change the original logic as little as possible.

If we just put all the add index jobs at the end of the default list, what the worst-case result would be?

@coocood
Added an index that should not be added.
e.g.
original order: drop column col1 -> add index idx1(col1) should be failed -> add column col1
current order: drop column col1 -> add column col1 -> add index idx1(col1) successful

coocood · 2018-03-31T08:27:55Z

@zimulala
Would it be safe and simpler if we just move all the jobs in AddIdx queue to default queue?

coocood · 2018-03-31T07:55:57Z

ddl/schema_test.go

@@ -221,6 +221,7 @@ LOOP:
 case <-ticker.C:
 d.Stop()
 d.start(context.Background())
+ time.Sleep(time.Millisecond * 20)


Why add sleep?

Because we add check add index list, it makes processing the first job a bit slower.

coocood · 2018-03-31T08:10:35Z

meta/meta.go

- mDDLJobHistoryKey = []byte("DDLJobHistory")
- mDDLJobReorgKey = []byte("DDLJobReorg")
+ mDDLJobListKey = []byte("DDLJobList")
+ mAddIdxDDLJobListKey = []byte("AddIdxDDLJobList")


How about DDLJobAddIdxList?

shenli · 2018-04-02T08:59:26Z

ddl/ddl_worker.go

@@ -191,7 +193,7 @@ func (d *ddl) getHistoryDDLJob(id int64) (*model.Job, error) {
 return job, errors.Trace(err)
 }

-func (d *ddl) handleDDLJobQueue() error {
+func (d *ddl) handleDDLJobQueue(shouldCleanJobs *bool) error {


Add a comment for the new parameter.

Use return value to control the outer for loop.

zimulala · 2018-04-03T07:40:47Z

PTAL @winkyao @coocood @shenli

coocood · 2018-04-03T08:08:34Z

ddl/ddl_worker.go

 if err != nil {
 log.Errorf("[ddl] handle ddl job err %v", errors.ErrorStack(err))
+ } else {
+ shouldCleanJobs = !isCleaned


It's possible that the shouldCleanJobs is false, but isCleaned return false, then we will clean job again.

If err is nil, can we safely set shouldCleanJobs to false?
Then we can remove the bool return value.

coocood · 2018-04-03T09:08:34Z

LGTM

zimulala · 2018-04-03T10:21:01Z

PTAL @winkyao @shenli

zimulala · 2018-04-04T02:38:17Z

PTAL @winkyao @shenli

XuHuaiyu

rest LGTM

XuHuaiyu · 2018-04-04T06:50:11Z

ddl/ddl_worker.go

+ continue
+ }
+
+ // When the job not in "none" state, we need to rollback it.


"none" and "public" ?

XuHuaiyu · 2018-04-04T07:20:43Z

ddl/ddl_worker.go

@@ -52,6 +52,8 @@ func (d *ddl) onDDLWorker() {
 metrics.PanicCounter.WithLabelValues(metrics.LabelDDL).Inc()
 }
 }()
+
+ shouldCleanJobs := true


may add a comment for this to explain why we need this.

winkyao

LGTM

comment to be addressed

zimulala · 2018-04-04T08:59:27Z

/run-all-tests

zimulala · 2018-04-04T09:50:28Z

/integration-common-test

zimulala · 2018-04-04T09:51:07Z

/run-integration-common-test

ddl,meta: clean the jobs in adding index list

a385265

zhexuany reviewed Mar 28, 2018

View reviewed changes

ddl: update the comment

d0e3bab

zimulala force-pushed the ddl-compatibility branch from 620c88f to d0e3bab Compare March 28, 2018 02:52

ddl: make test stable

adce88d

zimulala force-pushed the ddl-compatibility branch from a5d87d9 to adce88d Compare March 29, 2018 06:10

zimulala added the DDL label Mar 30, 2018

winkyao reviewed Mar 30, 2018

View reviewed changes

coocood reviewed Mar 31, 2018

View reviewed changes

shenli reviewed Apr 2, 2018

View reviewed changes

zimulala mentioned this pull request Apr 2, 2018

*: support set relation job #6206

Merged

zimulala added 2 commits April 3, 2018 15:23

*: address comments

739f7a2

Merge branch 'master' into zimuxia/ddl-compatibility

f5c4444

zimulala force-pushed the ddl-compatibility branch from c1515e7 to f5c4444 Compare April 3, 2018 07:24

coocood reviewed Apr 3, 2018

View reviewed changes

ddl: address comments

4254b4e

zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 3, 2018

XuHuaiyu reviewed Apr 4, 2018

View reviewed changes

winkyao previously approved these changes Apr 4, 2018

View reviewed changes

zimulala added 2 commits April 4, 2018 15:51

ddl: address comments

a54cfe1

ddl: tiny update

e40cd70

zimulala added status/LGT2 Indicates that a PR has LGTM 2. all-tests-passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 4, 2018

winoros approved these changes Apr 4, 2018

View reviewed changes

winoros and others added 2 commits April 4, 2018 18:35

Merge branch 'master' into ddl-compatibility

5659835

Merge branch 'master' into ddl-compatibility

9d99aad

zimulala merged commit cb04e97 into pingcap:master Apr 4, 2018

zimulala deleted the ddl-compatibility branch April 4, 2018 11:04

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl, meta: clean the jobs in adding index queue #6161

ddl, meta: clean the jobs in adding index queue #6161

zimulala commented Mar 27, 2018 •

edited

Loading

zhexuany Mar 28, 2018

zimulala commented Mar 28, 2018

zimulala commented Mar 28, 2018

zimulala commented Mar 29, 2018

zhexuany commented Mar 29, 2018

zimulala commented Mar 30, 2018

winkyao Mar 30, 2018

zimulala Apr 3, 2018

winkyao Mar 30, 2018

winkyao Mar 30, 2018

zimulala Apr 2, 2018 •

edited

Loading

coocood Apr 2, 2018 •

edited

Loading

zimulala Apr 2, 2018

coocood commented Mar 31, 2018

coocood Mar 31, 2018

zimulala Apr 2, 2018

coocood Mar 31, 2018

shenli Apr 2, 2018

shenli Apr 2, 2018

zimulala commented Apr 3, 2018

coocood Apr 3, 2018

coocood Apr 3, 2018

coocood commented Apr 3, 2018

zimulala commented Apr 3, 2018

zimulala commented Apr 4, 2018

XuHuaiyu left a comment

XuHuaiyu Apr 4, 2018

XuHuaiyu Apr 4, 2018

winkyao left a comment

zimulala commented Apr 4, 2018

zimulala commented Apr 4, 2018

zimulala commented Apr 4, 2018

ddl, meta: clean the jobs in adding index queue #6161

ddl, meta: clean the jobs in adding index queue #6161

Conversation

zimulala commented Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

zimulala commented Mar 28, 2018

zimulala commented Mar 28, 2018

zimulala commented Mar 29, 2018

zhexuany commented Mar 29, 2018

zimulala commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

coocood Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Apr 3, 2018

zimulala commented Apr 3, 2018

zimulala commented Apr 4, 2018

XuHuaiyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

zimulala commented Apr 4, 2018

zimulala commented Apr 4, 2018

zimulala commented Apr 4, 2018

zimulala commented Mar 27, 2018 •

edited

Loading

zimulala Apr 2, 2018 •

edited

Loading

coocood Apr 2, 2018 •

edited

Loading