syncer(dm): init schemaTracker when syncer run #6052

buchuitoudegou · 2022-06-27T07:56:39Z

What problem does this PR solve?

Issue Number: close #6014

What is changed and how it works?

The validator will get the old schemaTracker because the syncer re-builds the schemaTracker every time it starts. To avoid that, I'm about to decouple the creating and initialization of the schemaTracker so that the syncer will only initialize the schemaTracker when it starts to run and the schemaTracker remains the same instance in the whole lifecycle of the task.

create dump schemaTracker when initializing the syncer
initialize the schemaTracker when running

Check List

Tests

Unit test

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

`None`

ti-chi-bot · 2022-06-27T07:56:40Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
lance6716

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

buchuitoudegou · 2022-06-27T07:56:47Z

/area dm

buchuitoudegou · 2022-06-27T08:49:01Z

/run-dm-integration-test

buchuitoudegou · 2022-06-27T09:06:15Z

schemaTracker remains the same instance in the whole lifecycle of the task

this will cause #5344 and #4187

The schemaTracker can still be Close'd every time the task is paused, where the persistent storage is removed. Just avoid creating a new instance.

tiflow/dm/pkg/schema/tracker.go

Line 400 in 9035b2f

return os.RemoveAll(tr.storePath)

buchuitoudegou · 2022-06-27T09:06:44Z

/run-dm-integration-test

lance6716

And also can we add a test of "two tracker"?
rest lgtm

lance6716 · 2022-06-27T09:57:43Z

dm/pkg/schema/tracker.go

@@ -99,16 +99,25 @@ type DownstreamTableInfo struct {
 	WhereHandle *sqlmodel.WhereHandle
 }

-// NewTracker creates a new tracker. `sessionCfg` will be set as tracker's session variables if specified, or retrieve
+// NewDumpTracker simply returns an empty Tracker,


what's the meaning of Dump? Syncer can run without dump unit.

Will rename it.

lance6716 · 2022-06-27T09:59:49Z

dm/syncer/syncer.go

@@ -1696,7 +1698,13 @@ func (s *Syncer) Run(ctx context.Context) (err error) {
 		}
 	}

-	s.schemaTracker, err = schema.NewTracker(ctx, s.cfg.Name, s.cfg.To.Session, s.downstreamTrackConn, s.tctx.L())
+	if s.schemaTracker == nil {
+		s.schemaTracker, err = schema.NewTracker(ctx, s.cfg.Name, s.cfg.To.Session, s.downstreamTrackConn, s.tctx.L())


this is only to make test happy? we can left a comment

Yes. some tests set the schemaTracker themselves. Will leave a comment.

buchuitoudegou · 2022-06-27T10:32:42Z

/cc @D3Hunter

D3Hunter · 2022-06-27T11:03:00Z

dm/pkg/schema/tracker.go

 	ctx context.Context,
 	task string,
 	sessionCfg map[string]string,
 	downstreamConn *dbconn.DBConn,
 	logger log.Logger,
-) (*Tracker, error) {
+) error {
+	if tr == nil {


when will tr be nil?

if it's for test, we can change test instead

This line is copied from the old version. I remember it will let the load_task IT fail if deleted (probably this test case tries to pause-task before initializing the syncer). will take a look tomorrow.

D3Hunter · 2022-06-27T11:12:20Z

dm/pkg/schema/tracker.go

-		dsTracker: dsTracker,
-	}, nil
+// NewTracker creates a new tracker. It's preserved for test.
+func NewTracker(


prefer to name NewDumpTracker as NewTracker, and remove this one.

for test, we change them to Init

Maybe leaving a function invoking NewTracker and subsequently Init is better, so as to make the test more succinct.

then name it explicitly as something like NewTrackerForTest. NewTracker is how we name normal ctor

IDE should be able to easily rename them all

buchuitoudegou · 2022-06-29T03:57:58Z

/run-dm-integration-test

codecov-commenter · 2022-06-29T04:47:59Z

Codecov Report

Merging #6052 (8dceb81) into master (b1ce2c9) will decrease coverage by 3.1611%.
The diff coverage is 44.0251%.

Flag	Coverage Δ
cdc	`?`
dm	`51.9370% <43.8461%> (-0.0359%)`	⬇️
engine	`62.2086% <44.0922%> (-0.2319%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #6052        +/-   ##
================================================
- Coverage   57.2815%   54.1204%   -3.1612%     
================================================
  Files           680        399       -281     
  Lines         80230      50055     -30175     
================================================
- Hits          45957      27090     -18867     
+ Misses        30009      20302      -9707     
+ Partials       4264       2663      -1601

D3Hunter · 2022-06-29T06:00:15Z

dm/pkg/schema/tracker.go

+	tr.Lock()
+	defer tr.Unlock()


now this lock is used in 3 places, please refine comments in Tracker definition

buchuitoudegou · 2022-06-29T06:57:56Z

/run-verify
/run-leak-test
/run-dm-integration-test

lance6716 · 2022-06-29T10:04:39Z

/merge

ti-chi-bot · 2022-06-29T10:04:42Z

This pull request has been accepted and is ready to merge.

Commit hash: 77cfbca

buchuitoudegou · 2022-06-29T16:31:41Z

/run-dm-integration-test

buchuitoudegou · 2022-06-30T05:15:03Z

/run-dm-integration-test

buchuitoudegou · 2022-06-30T06:29:37Z

/run-dm-integration-test

) * syncer(dm): init schemaTracker when syncer run (#6052) close #6014 * cdc: upgrade pebble and stabilize a case (#6116) close #5798 * meta(engine): Resourcemeta support job level isolation (#5817) close #5816 * owner(cdc): clean up admin jobs on async close (#6129) close #6128 * pkg/errctx(engine): avoid creating goroutinue in errctx (#6061) ref #6013 * cdc,pkg: accommodate systemd TimeoutStopSec and k8s terminationGracePeriodSeconds (#6097) * cdc,pkg: accommodate systemd TimeoutStopSec and k8s terminationGracePeriodSeconds * cdc: correct drain capture doc Signed-off-by: Neil Shen <[email protected]> * cdc: drain waits for owner resign and table move out Signed-off-by: Neil Shen <[email protected]> * tp(cdc): make drain capture scheduler aware of capture state Signed-off-by: Neil Shen <[email protected]> * cdc: remove verbose log and set liveness stop after resign owner Signed-off-by: Neil Shen <[email protected]> * cdc: skip owner resign when there is only one capture Signed-off-by: Neil Shen <[email protected]> * fix ut * apply comments * fix integration test * fix lint Co-authored-by: Obliviate <[email protected]> Co-authored-by: qupeng <[email protected]> Co-authored-by: maxshuang <[email protected]> Co-authored-by: Neil Shen <[email protected]> Co-authored-by: Yujie Xia <[email protected]>

init schemaTracker when syncer run

5e38c28

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 27, 2022

ti-chi-bot added the area/dm Issues or PRs related to DM. label Jun 27, 2022

buchuitoudegou marked this pull request as ready for review June 27, 2022 07:56

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 27, 2022

fix: nil ptr exception

ac6c120

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2022

This comment was marked as resolved.

Sign in to view

lance6716 reviewed Jun 27, 2022

View reviewed changes

ti-chi-bot requested a review from D3Hunter June 27, 2022 10:32

D3Hunter reviewed Jun 27, 2022

View reviewed changes

fix comment

8dceb81

D3Hunter reviewed Jun 29, 2022

View reviewed changes

update tracker comment

77cfbca

D3Hunter approved these changes Jun 29, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 29, 2022

lance6716 approved these changes Jun 29, 2022

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jun 29, 2022

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 29, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 29, 2022

ti-chi-bot added 2 commits June 29, 2022 18:04

Merge branch 'master' into fix-6014

5a82679

Merge branch 'master' into fix-6014

a713b5f

ti-chi-bot added 2 commits June 30, 2022 11:54

Merge branch 'master' into fix-6014

85a9541

Merge branch 'master' into fix-6014

73ddbc2

Merge branch 'master' into fix-6014

fe59d73

ti-chi-bot merged commit 4839e39 into pingcap:master Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syncer(dm): init schemaTracker when syncer run #6052

syncer(dm): init schemaTracker when syncer run #6052

buchuitoudegou commented Jun 27, 2022

ti-chi-bot commented Jun 27, 2022 •

edited

Loading

buchuitoudegou commented Jun 27, 2022

buchuitoudegou commented Jun 27, 2022

This comment was marked as resolved.

buchuitoudegou commented Jun 27, 2022

buchuitoudegou commented Jun 27, 2022

lance6716 left a comment

lance6716 Jun 27, 2022

buchuitoudegou Jun 27, 2022

buchuitoudegou Jun 29, 2022

lance6716 Jun 27, 2022

buchuitoudegou Jun 27, 2022

buchuitoudegou Jun 29, 2022

buchuitoudegou commented Jun 27, 2022

D3Hunter Jun 27, 2022

buchuitoudegou Jun 27, 2022

D3Hunter Jun 27, 2022

buchuitoudegou Jun 27, 2022

D3Hunter Jun 28, 2022

lance6716 Jun 28, 2022

buchuitoudegou Jun 29, 2022

buchuitoudegou commented Jun 29, 2022

codecov-commenter commented Jun 29, 2022

D3Hunter Jun 29, 2022

buchuitoudegou Jun 29, 2022

buchuitoudegou commented Jun 29, 2022

lance6716 commented Jun 29, 2022

ti-chi-bot commented Jun 29, 2022

buchuitoudegou commented Jun 29, 2022

buchuitoudegou commented Jun 30, 2022

buchuitoudegou commented Jun 30, 2022

syncer(dm): init schemaTracker when syncer run #6052

syncer(dm): init schemaTracker when syncer run #6052

Conversation

buchuitoudegou commented Jun 27, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot commented Jun 27, 2022 • edited Loading

buchuitoudegou commented Jun 27, 2022

buchuitoudegou commented Jun 27, 2022

This comment was marked as resolved.

buchuitoudegou commented Jun 27, 2022

buchuitoudegou commented Jun 27, 2022

lance6716 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchuitoudegou commented Jun 27, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchuitoudegou commented Jun 29, 2022

codecov-commenter commented Jun 29, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchuitoudegou commented Jun 29, 2022

lance6716 commented Jun 29, 2022

ti-chi-bot commented Jun 29, 2022

buchuitoudegou commented Jun 29, 2022

buchuitoudegou commented Jun 30, 2022

buchuitoudegou commented Jun 30, 2022

ti-chi-bot commented Jun 27, 2022 •

edited

Loading