ddl: handle store closed in doDDLJob #18844

lance6716 · 2020-07-29T03:28:48Z

What problem does this PR solve?

Issue Number: close #18714 , pingcap/dm#792

Problem Summary: add an exit branch in doDDLJob

What is changed and how it works?

What's Changed:

detected d.ctx.Done() in doDDLJob, which may caused by worker closed, store closed, ..., to avoid deadloop. Note now doDDLJob will fail immediately when worker closed, so changed some 5-year-old test

How it Works:

add an exit branch in doDDLJob, add a unit test to verify

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Unit test

Side effects

Release note

No release note

codecov · 2020-07-31T05:48:12Z

Codecov Report

Merging #18844 into master will decrease coverage by 0.0289%.
The diff coverage is 100.0000%.

@@               Coverage Diff                @@
##             master     #18844        +/-   ##
================================================
- Coverage   79.1739%   79.1450%   -0.0290%     
================================================
  Files           548        548                
  Lines        148545     147883       -662     
================================================
- Hits         117609     117042       -567     
+ Misses        21449      21370        -79     
+ Partials       9487       9471        -16

lance6716 · 2020-07-31T05:50:06Z

PTAL @zimulala

lance6716 · 2020-08-05T02:05:15Z

PTAL @zimulala

lance6716 · 2020-08-13T02:26:24Z

PTAL @AilinKid @zimulala

AilinKid

Rest LGTM

ddl/ddl.go

AilinKid · 2020-08-13T03:13:41Z

/run-all-tests

AilinKid

LGTM

csuzhangxc · 2020-08-20T07:54:59Z

@zimulala PTAL, thanks!

zimulala · 2020-08-20T08:12:16Z

ddl/stat_test.go

 			c.Assert(s.getDDLSchemaVer(c, d), GreaterEqual, ver)
-			d.restartWorkers(context.Background())


Why remove this test? Will deleting this test reduce test scenarios?

d.Stop()will call d.cancel(), which will trigger doDDLJob quit

We can replace done <- d.doDDLJob(ctx, job) with checking the job in history. Instead of removing these test scenarios.

session/session_test.go

zimulala · 2020-08-20T08:14:06Z

session/session_test.go

+
+	c.Assert(failpoint.Enable("github.com/pingcap/tidb/ddl/storeCloseInLoop", `return(2)`), IsNil)
+	go func() {
+		time.Sleep(time.Second)


I think use Sleep and go will make the test unstable and "sleep 2s" make the test run a long test.

lance6716 · 2020-08-24T01:53:38Z

/run-all-tests

lance6716 · 2020-08-24T07:03:04Z

/run-all-tests

lance6716 · 2020-08-24T07:07:21Z

seems an error irrelevant to this PR could be reproduce:

run test [alter_table1] err: sql:ALTER TABLE m1 ENABLE KEYS, ALGORITHM= INPLACE;: failed to run query \n\"ALTER TABLE m1 ENABLE KEYS, ALGORITHM= INPLACE;\" \n around line 317, \nwe need(158):\nALTER TABLE m1 ENABLE KEYS, ALGORITHM= INPLACE;\nALTER TABLE t1 DROP INDEX i1;\nALTER TABLE t1 DROP INDEX i2;\nALTER TABLE t1 DROP INDEX i3;\nALTER TABLE t1 DROP \nbut got(158):\nALTER TABLE m1 ENABLE KEYS, ALGORITHM= INPLACE;\nError 1846: ALGORITHM=INPLACE is not supported. Reason: Cannot alter table by INPLACE. Try ALGORITHM=INSTANT.\n\n

lance6716 · 2020-08-25T01:56:33Z

PTAL @zimulala

This reverts commit df93c43.

AilinKid

~~LGTM~~

lance6716 · 2020-08-28T03:04:15Z

~~LGTM~~

Sorry there're some concurrency problem still, I'll fix it when I have some spare time 😢

lance6716 · 2020-08-28T03:51:09Z

ddl/restart_test.go

+// It only starts the original workers.
+func (d *ddl) restartWorkers(ctx context.Context) {
+	d.cancel()
+	d.wg.Wait()


added these two lines⬆️

lance6716 · 2020-08-28T08:50:30Z

now CI is failed because Sending interrupt signal to process. waiting for more advice on code @zimulala

lance6716 · 2020-08-28T10:05:02Z

/run-all-tests

lance6716 · 2020-08-29T13:28:40Z

Oh finally CI passed

zimulala

LGTM

ti-srebot · 2020-09-01T03:21:01Z

/run-all-tests

tisonkun · 2021-10-07T08:42:46Z

ddl/ddl.go

@@ -488,9 +490,17 @@ func (d *ddl) doDDLJob(ctx sessionctx.Context, job *model.Job) error {
 		metrics.HandleJobHistogram.WithLabelValues(job.Type.String(), metrics.RetLabel(err)).Observe(time.Since(startTime).Seconds())
 	}()
 	for {
+		failpoint.Inject("storeCloseInLoop", func(_ failpoint.Value) {
+			d.cancel()


We may better call d.Stop here; otherwise clean up jobs like for del range won't be triggered.

ddl: handle store closed in doDDLJob

04d1d47

github-actions bot added the sig/sql-infra SIG: SQL Infra label Jul 29, 2020

lance6716 marked this pull request as draft July 29, 2020 05:56

lance6716 added 4 commits July 31, 2020 11:39

fix typo

d121e79

Merge branch 'master' into fix-dm-log

e4ca463

fix test

c777d62

fix test

2b023da

lance6716 marked this pull request as ready for review July 31, 2020 05:44

lance6716 mentioned this pull request Jul 31, 2020

worker aggressively output error log pingcap/dm#792

Closed

github-actions bot added the component/DDL label Aug 4, 2020

AndreMouche removed the component/DDL label Aug 11, 2020

AilinKid reviewed Aug 13, 2020

View reviewed changes

ddl/ddl.go Outdated Show resolved Hide resolved

Merge branch 'master' into fix-dm-log

dc277bd

address comment

c0a61c8

AilinKid reviewed Aug 13, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 13, 2020

zimulala reviewed Aug 20, 2020

View reviewed changes

address comments

8c9f2e5

lance6716 added 2 commits August 24, 2020 18:00

Merge branch 'master' into fix-dm-log

4326855

fix test

c70a8ae

lance6716 added 4 commits August 27, 2020 20:13

Revert "remove used variable"

dd787f2

This reverts commit df93c43.

modify comment

04297d4

try fix deadlock

9dc8c5f

try fix deadlock

8f2f776

AilinKid reviewed Aug 28, 2020

View reviewed changes

Merge branch 'master' into fix-dm-log

158656f

lance6716 changed the title ~~ddl: handle store closed in doDDLJob~~ [WIP] ddl: handle store closed in doDDLJob Aug 28, 2020

try fix test

d0fa2f0

lance6716 commented Aug 28, 2020

View reviewed changes

Merge branch 'master' into fix-dm-log

28d5ceb

lance6716 changed the title ~~[WIP] ddl: handle store closed in doDDLJob~~ ddl: handle store closed in doDDLJob Aug 28, 2020

Merge branch 'master' into fix-dm-log

f3e9304

zimulala approved these changes Sep 1, 2020

View reviewed changes

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 1, 2020

ti-srebot approved these changes Sep 1, 2020

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 1, 2020

Merge branch 'master' into fix-dm-log

8f52186

zimulala added the status/can-merge Indicates a PR has been approved by a committer. label Sep 1, 2020

ti-srebot merged commit f82e532 into pingcap:master Sep 1, 2020

lance6716 deleted the fix-dm-log branch September 1, 2020 03:29

lance6716 mentioned this pull request Sep 1, 2020

dep: update tidb (fix print massive log) and parser pingcap/dm#968

Merged

ti-srebot mentioned this pull request Sep 2, 2020

dep: update tidb (fix print massive log) and parser (#968) pingcap/dm#976

Closed

lance6716 mentioned this pull request Sep 5, 2020

ddl, test: fix deadloop in test about doDDLJob #19815

Merged

lance6716 added a commit to lance6716/tidb that referenced this pull request Sep 5, 2020

revert some change introduced in pingcap#18844

9151ba8

tisonkun reviewed Oct 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: handle store closed in doDDLJob #18844

ddl: handle store closed in doDDLJob #18844

lance6716 commented Jul 29, 2020 •

edited

Loading

codecov bot commented Jul 31, 2020

lance6716 commented Jul 31, 2020

lance6716 commented Aug 5, 2020

lance6716 commented Aug 13, 2020

AilinKid left a comment

AilinKid commented Aug 13, 2020

AilinKid left a comment

csuzhangxc commented Aug 20, 2020

zimulala Aug 20, 2020

lance6716 Aug 20, 2020

zimulala Aug 20, 2020

zimulala Aug 20, 2020

lance6716 commented Aug 24, 2020

lance6716 commented Aug 24, 2020

lance6716 commented Aug 24, 2020

lance6716 commented Aug 25, 2020

AilinKid left a comment •

edited

Loading

lance6716 commented Aug 28, 2020 •

edited

Loading

lance6716 Aug 28, 2020

lance6716 commented Aug 28, 2020

lance6716 commented Aug 28, 2020

lance6716 commented Aug 29, 2020

zimulala left a comment

ti-srebot commented Sep 1, 2020

tisonkun Oct 7, 2021

		c.Assert(s.getDDLSchemaVer(c, d), GreaterEqual, ver)
		d.restartWorkers(context.Background())

ddl: handle store closed in doDDLJob #18844

ddl: handle store closed in doDDLJob #18844

Conversation

lance6716 commented Jul 29, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

codecov bot commented Jul 31, 2020

Codecov Report

lance6716 commented Jul 31, 2020

lance6716 commented Aug 5, 2020

lance6716 commented Aug 13, 2020

AilinKid left a comment

Choose a reason for hiding this comment

AilinKid commented Aug 13, 2020

AilinKid left a comment

Choose a reason for hiding this comment

csuzhangxc commented Aug 20, 2020

zimulala Aug 20, 2020

Choose a reason for hiding this comment

lance6716 Aug 20, 2020

Choose a reason for hiding this comment

zimulala Aug 20, 2020

Choose a reason for hiding this comment

zimulala Aug 20, 2020

Choose a reason for hiding this comment

lance6716 commented Aug 24, 2020

lance6716 commented Aug 24, 2020

lance6716 commented Aug 24, 2020

lance6716 commented Aug 25, 2020

AilinKid left a comment • edited Loading

Choose a reason for hiding this comment

lance6716 commented Aug 28, 2020 • edited Loading

lance6716 Aug 28, 2020

Choose a reason for hiding this comment

lance6716 commented Aug 28, 2020

lance6716 commented Aug 28, 2020

lance6716 commented Aug 29, 2020

zimulala left a comment

Choose a reason for hiding this comment

ti-srebot commented Sep 1, 2020

tisonkun Oct 7, 2021

Choose a reason for hiding this comment

lance6716 commented Jul 29, 2020 •

edited

Loading

AilinKid left a comment •

edited

Loading

lance6716 commented Aug 28, 2020 •

edited

Loading