ddl: sync schema version using watch, notify sessions on owner node by job id #53217

D3Hunter · 2024-05-13T06:36:31Z

What problem does this PR solve?

Issue Number: ref #53246

Problem Summary:

What changed and how does it work?

sync schema version using etcd watch
notify sessions on owner by job id to avoid triggering a wrong session when we run DDLs using multiple connections.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

1 pd/3 tikv, 1 tidb. Run on owner node. 20-30% faster

create table

thread=1, 500 tables
1m3.675s(127ms per table) -> 47.732s(95ms per table)

Thread=4, 2000 tables
3m8.39s(94ms per table) -> 2m20.273s(70ms per table)

Thread=16, 2000 tables
2m49.304s(85ms per table) -> 2m42.364s(81ms per table)

create database

thread=1, 500 databases
1m0.177s(120ms per op) -> 47.345s(95ms per op)

Thread=4, 2000 databases
3m35.87s(108ms per op) -> 2m29.758s(75ms per op)

1 pd/3 tikv, 8 tidb. Run on owner node, create table. 42% faster

thread=1, 500 tables
1m6s(132ms per table) -> 37.999s(76ms per table)

No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

log log

ti-chi-bot · 2024-05-13T06:36:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2024-05-13T06:36:46Z

Hi @D3Hunter. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

This reverts commit 817c4d6.

D3Hunter · 2024-05-14T06:16:56Z

/test all

tiprow · 2024-05-14T06:17:18Z

@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

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.

D3Hunter · 2024-05-14T06:19:10Z

/test all

tiprow · 2024-05-14T06:19:31Z

@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

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.

D3Hunter · 2024-05-14T06:46:54Z

/test all

tiprow · 2024-05-14T06:47:19Z

@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

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.

D3Hunter · 2024-05-14T07:04:29Z

/retest

tiprow · 2024-05-14T07:04:51Z

@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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 · 2024-05-14T07:05:59Z

Codecov Report

Attention: Patch coverage is 87.33032% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 73.9039%. Comparing base (43fd1b2) to head (738f11c).
Report is 60 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #53217        +/-   ##
================================================
+ Coverage   72.4365%   73.9039%   +1.4674%     
================================================
  Files          1492       1524        +32     
  Lines        428995     445635     +16640     
================================================
+ Hits         310749     329342     +18593     
+ Misses        98991      96177      -2814     
- Partials      19255      20116       +861

Flag	Coverage Δ
integration	`49.1316% <68.3257%> (?)`
unit	`71.3396% <80.5429%> (+0.0069%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`42.0474% <ø> (+0.6239%)`	⬆️

lance6716

rest lgtm except for MDL part, I need more time to learn MDL behaviour 😂

lance6716 · 2024-05-16T07:35:06Z

pkg/ddl/syncer/syncer.go

@@ -126,16 +128,98 @@ type SchemaSyncer interface {
 	// the latest schema version. (exclude the isolated TiDB)
 	// It returns until all servers' versions are equal to the latest version.
 	OwnerCheckAllVersions(ctx context.Context, jobID int64, latestVer int64) error
+	// SyncJobSchemaVerLoop syncs the schema versions on all TiDB nodes for DDL jobs.
+	SyncJobSchemaVerLoop(ctx context.Context)


can we move it inside NewSchemaSyncer?

it's part of DDL, start background routines in ddl.Start should be more clear.

lance6716 · 2024-05-16T07:38:02Z

pkg/ddl/syncer/syncer.go

+	v.Lock()
+	defer v.Unlock()
+	if ok := fn(v.nodeVersions); !ok {
+		// onceMatchFn must be nil before.


maybe move it to function comments, so when I hover the cursor above the function in IDE I can see the comments.

lance6716 · 2024-05-16T07:46:12Z

pkg/ddl/syncer/syncer.go

+		s.handleJobSchemaVerKV(oneKV, mvccpb.PUT)
+	}
+	s.mu.Lock()
+	// we might miss some DELETE events during retry, some items might be emptyAndNotUsed, remove them.


every resp of line 498 is a snapshot, we can always remove all the old data and apply the resp, no need to have two removing

lance6716 · 2024-05-16T07:53:45Z

pkg/ddl/ddl.go

@@ -1205,13 +1233,18 @@ func (d *ddl) DoDDLJob(ctx sessionctx.Context, job *model.Job) error {
 		recordLastDDLInfo(ctx, historyJob)
 	}()
 	i := 0
+	notifyCh, ok := d.getJobDoneCh(job.ID)
+	if !ok {
+		// shouldn't happen, just give it a dummy one


it's OK to select from nil channel https://golangbyexample.com/select-with-nil-channel-golang/

ti-chi-bot · 2024-05-17T06:11:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, wjhuang2016

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:

~~pkg/ddl/OWNERS~~ [wjhuang2016]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-05-17T06:11:25Z

[LGTM Timeline notifier]

Timeline:

2024-05-15 06:03:11.146044733 +0000 UTC m=+1633144.903180299: ☑️ agreed by wjhuang2016.
2024-05-17 06:11:24.18283088 +0000 UTC m=+1806437.939966473: ☑️ agreed by lance6716.

…y job id (pingcap#53217) ref pingcap#53246

D3Hunter added 3 commits May 13, 2024 10:59

change

8685798

time

817c4d6

log log

change

1de5d27

opt notify job, ut

0efbfd1

D3Hunter force-pushed the opt-sync-ver branch from b1fc457 to 0efbfd1 Compare May 13, 2024 14:35

D3Hunter added 4 commits May 13, 2024 23:56

change

970bbf4

Revert "time"

747e004

This reverts commit 817c4d6.

test

9260217

change

5a55db2

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels May 14, 2024

D3Hunter changed the title ~~ddl: optimize schema version sync~~ ddl: sync schema version using watch, notify sessions on owner node by job id May 14, 2024

D3Hunter mentioned this pull request May 14, 2024

optimize execution of general DDL #53246

Closed

18 tasks

bazel

64c914e

lint

8ba6b5f

test

a19a509

D3Hunter marked this pull request as ready for review May 14, 2024 07:28

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2024

D3Hunter added 2 commits May 14, 2024 16:10

comments

7f50f61

refine

040a45c

wjhuang2016 approved these changes May 15, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 15, 2024

D3Hunter requested a review from lance6716 May 16, 2024 07:26

lance6716 reviewed May 16, 2024

View reviewed changes

fix comments

738f11c

lance6716 approved these changes May 17, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 17, 2024

ti-chi-bot bot merged commit 5d990c6 into pingcap:master May 17, 2024
21 of 23 checks passed

D3Hunter deleted the opt-sync-ver branch May 17, 2024 06:38

terry1purcell pushed a commit to terry1purcell/tidb that referenced this pull request May 17, 2024

ddl: sync schema version using watch, notify sessions on owner node b…

f684ddc

…y job id (pingcap#53217) ref pingcap#53246

RidRisR pushed a commit to RidRisR/tidb that referenced this pull request May 23, 2024

ddl: sync schema version using watch, notify sessions on owner node b…

8d262fa

…y job id (pingcap#53217) ref pingcap#53246

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: sync schema version using watch, notify sessions on owner node by job id #53217

ddl: sync schema version using watch, notify sessions on owner node by job id #53217

D3Hunter commented May 13, 2024 •

edited

Loading

ti-chi-bot bot commented May 13, 2024

tiprow bot commented May 13, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

codecov bot commented May 14, 2024 •

edited

Loading

lance6716 left a comment

lance6716 May 16, 2024

D3Hunter May 16, 2024

lance6716 May 16, 2024

lance6716 May 16, 2024

D3Hunter May 17, 2024

lance6716 May 16, 2024

ti-chi-bot bot commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

ddl: sync schema version using watch, notify sessions on owner node by job id #53217

ddl: sync schema version using watch, notify sessions on owner node by job id #53217

Conversation

D3Hunter commented May 13, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented May 13, 2024

tiprow bot commented May 13, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

D3Hunter commented May 14, 2024

tiprow bot commented May 14, 2024

codecov bot commented May 14, 2024 • edited Loading

Codecov Report

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 May 16, 2024

Choose a reason for hiding this comment

D3Hunter May 16, 2024

Choose a reason for hiding this comment

lance6716 May 16, 2024

Choose a reason for hiding this comment

lance6716 May 16, 2024

Choose a reason for hiding this comment

D3Hunter May 17, 2024

Choose a reason for hiding this comment

lance6716 May 16, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

[LGTM Timeline notifier]

D3Hunter commented May 13, 2024 •

edited

Loading

codecov bot commented May 14, 2024 •

edited

Loading