DM: add checkpoint test for lightning-loader #3220

Little-Wallace · 2021-11-01T08:44:19Z

Signed-off-by: Little-Wallace [email protected]

What problem does this PR solve?

What is changed and how it works?

Add checkpoint test for lightning tidb-backend. I hope we can use tidb-backend to replace loader of dm.

Check List

Tests

Integration test

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to update key monitor metrics in both TiCDC document and official document

Release note

None

Signed-off-by: Little-Wallace <[email protected]>

ti-chi-bot · 2021-11-01T08:44:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996
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.

CLAassistant · 2021-11-01T08:44:23Z

All committers have signed the CLA.

dm/loader/lightning.go

dm/cmd/dm-worker/main.go

dm/loader/lightning.go

Ehco1996 · 2021-11-02T00:33:16Z

dm/loader/lightning.go

-		err = l.core.RunOnce(taskCtx, cfg, nil)
+		err = l.runLightning(ctx, cfg)
+		if err == nil {
+			err = lightning.CheckpointRemove(ctx, cfg, "all")


maybe we can keep the checkpoint in target db and clear it when user start-task with flag remove-meta as syncer do

+1. In the current dm implementation, we should delay clean checkpoint data when stop task

we at least should make sure IsFreshTask returning false after a successful loading, so it needs we don't remove lightning checkpoint or let IsFreshTask inspecting LightningCheckpointList

dm/tests/lightning_load_task/conf/dm-task.yaml

Signed-off-by: Little-Wallace <[email protected]>

Ehco1996 · 2021-11-04T02:54:30Z

/run-dm-integration-tests

dm/loader/lightning.go

lance6716 · 2021-11-04T07:16:58Z

dm/loader/lightning.go

@@ -150,7 +180,8 @@ func (l *LightningLoader) restore(ctx context.Context) error {
 		}
 		cfg.Routes = l.cfg.RouteRules
 		cfg.Checkpoint.Driver = lcfg.CheckpointDriverMySQL
-		cfg.Checkpoint.Schema = config.TiDBLightningCheckpointPrefix + dbutil.ColumnName(l.workerName)
+		cfg.Checkpoint.Schema = config.TiDBLightningCheckpointPrefix + dbutil.TableName(l.workerName, l.cfg.Name)


I'm not sure why we need dbutil.TableName, because in NewMySQLCheckpointsDB of lightning it will do escaping for backquotes 🤔

can we only use config.TiDBLightningCheckpointPrefix + l.workerName + l.cfg.Name as database name?

It seems that can not pass test

dm/loader/lightning.go

Signed-off-by: Little-Wallace <[email protected]>

Ehco1996 · 2021-11-08T02:04:14Z

ping @Little-Wallace

Signed-off-by: Little-Wallace <[email protected]>

lance6716

rest lgtm

I'll implement remove-meta for lightning checkpoint in another PR

dm/loader/checkpoint.go

lance6716 · 2021-11-08T12:06:34Z

dm/loader/lightning.go

-		err = l.core.RunOnce(taskCtx, cfg, nil)
+		err = l.runLightning(ctx, cfg)
+		if err == nil {
+			err = lightning.CheckpointRemove(ctx, cfg, "all")


we at least should make sure IsFreshTask returning false after a successful loading, so it needs we don't remove lightning checkpoint or let IsFreshTask inspecting LightningCheckpointList

Signed-off-by: Little-Wallace <[email protected]>

lance6716

In future PR I'll check if we can use LightningCheckpointList to implement IsFreshTask, and implement --remove-meta for lightning loader

lance6716 · 2021-11-17T09:47:08Z

/run-all-tests

lance6716 · 2021-11-17T10:32:19Z

/run-all-tests

lance6716 · 2021-11-17T11:03:36Z

/run-all-tests

lance6716 · 2021-11-17T11:21:49Z

/run-all-tests

lance6716 · 2021-11-17T11:45:51Z

/run-all-tests

lance6716 · 2021-11-17T11:59:41Z

[2021-11-17T11:56:09.722Z] [2021/11/17 19:56:04.010 +08:00] [ERROR] [lightning.go:248] ["failed to runlightning"] [task=test] [unit=lightning-load] [error="invalid value / unknown config name: cluster"] [errorVerbose="invalid value / unknown config name: cluster\ngithub.com/pingcap/errors.AddStack\n\t/nfs/cache/mod/github.com/pingcap/[email protected]/errors.go:174\ngithub.com/pingcap/errors.Trace\n\t/nfs/cache/mod/github.com/pingcap/[email protected]/juju_adaptor.go:15\ngithub.com/pingcap/tidb/br/pkg/lightning/common.(*MySQLConnectParam).Connect\n\t/nfs/cache/mod/github.com/pingcap/[email protected]/br/pkg/lightning/common/util.go:70\ngithub.com/pingcap/tidb/br/pkg/lightning/restore.DBFromConfig\n\t/nfs/cache/mod/github.com/pingcap/[email protected]/br/pkg/lightning/restore/tidb.go:96\ngithub.com/pingcap/tidb/br/pkg/lightning.CleanupMetas\n\t/nfs/cache/mod/github.com/pingcap/[email protected]/br/pkg/lightning/lightning.go:752\ngithub.com/pingcap/tidb/br/pkg/lightning.CheckpointRemove\n\t/nfs/cache/mod/github.com/pingcap/[email protected]/br/pkg/lightning/lightning.go:739\ngithub.com/pingcap/ticdc/dm/loader.(*LightningLoader).restore\n\t/home/jenkins/agent/workspace/dm_ghpr_integration_test/go/src/github.com/pingcap/ticdc/dm/loader/lightning.go:240\ngithub.com/pingcap/ticdc/dm/loader.(*LightningLoader).Process\n\t/home/jenkins/agent/workspace/dm_ghpr_integration_test/go/src/github.com/pingcap/ticdc/dm/loader/lightning.go:292\ngithub.com/pingcap/ticdc/dm/loader.(*LightningLoader).Resume\n\t/home/jenkins/agent/workspace/dm_ghpr_integration_test/go/src/github.com/pingcap/ticdc/dm/loader/lightning.go:350\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1371"]

"cluster" is the TLS name 🤔

lance6716 · 2021-11-18T02:33:07Z

/run-all-tests

lance6716 · 2021-11-18T03:05:00Z

/run-dm-integration-test

lance6716 · 2021-11-18T03:26:16Z

/merge

DM-IT is passed https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/792/pipeline

ti-chi-bot · 2021-11-18T03:26:19Z

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

Commit hash: 7c5fd31

lance6716 · 2021-11-18T03:26:36Z

/unhold

lance6716 · 2021-11-18T03:27:38Z

/run-integration-test

codecov-commenter · 2021-11-18T04:05:05Z

Codecov Report

Merging #3220 (7c5fd31) into master (24030f8) will increase coverage by 1.3053%.
The diff coverage is 68.6951%.

@@               Coverage Diff                @@
##             master      #3220        +/-   ##
================================================
+ Coverage   56.5356%   57.8409%   +1.3053%     
================================================
  Files           211        234        +23     
  Lines         22798      23983      +1185     
================================================
+ Hits          12889      13872       +983     
- Misses         8598       8711       +113     
- Partials       1311       1400        +89

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2021-11-18T04:06:30Z

In response to a cherrypick label: new pull request created: #3512.

add test for checkpoint

8503156

Signed-off-by: Little-Wallace <[email protected]>

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 1, 2021

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 1, 2021

lonng reviewed Nov 1, 2021

View reviewed changes

dm/loader/lightning.go Outdated Show resolved Hide resolved

Ehco1996 reviewed Nov 2, 2021

View reviewed changes

glorv added the area/dm Issues or PRs related to DM. label Nov 2, 2021

Little-Wallace force-pushed the checkpoint branch from 9121b28 to c5bc67a Compare November 3, 2021 03:20

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 3, 2021

Little-Wallace added 2 commits November 3, 2021 11:22

address comment

c5bc67a

Signed-off-by: Little-Wallace <[email protected]>

address comment

54b0e52

Signed-off-by: Little-Wallace <[email protected]>

glorv added the needs-cherry-pick-release-5.3 Should cherry pick this PR to release-5.3 branch. label Nov 3, 2021

lance6716 reviewed Nov 4, 2021

View reviewed changes

Little-Wallace force-pushed the checkpoint branch from cbc358c to d5cad3f Compare November 4, 2021 10:14

ti-chi-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. labels Nov 4, 2021

drop checkpoint database

d5cad3f

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace added 2 commits November 8, 2021 11:42

fix warn

43ac644

Signed-off-by: Little-Wallace <[email protected]>

remove test for master remove meta

417c241

Signed-off-by: Little-Wallace <[email protected]>

lance6716 reviewed Nov 8, 2021

View reviewed changes

remove id

b6976e0

Signed-off-by: Little-Wallace <[email protected]>

lance6716 approved these changes Nov 11, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 11, 2021

Ehco1996 approved these changes Nov 11, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 11, 2021

ti-chi-bot removed status/can-merge Indicates a PR has been approved by a committer. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 17, 2021

fix make check

3b8ce78

lance6716 added 2 commits November 17, 2021 18:58

fix panic

17a4299

Merge branch 'master' into checkpoint

c6b16d2

fix CI

8bccbba

lance6716 force-pushed the checkpoint branch from 20accf8 to 8bccbba Compare November 17, 2021 11:45

lance6716 added 2 commits November 18, 2021 10:31

disable TLS test with lightning

e3c8bb4

Merge branch 'master' of github.com:pingcap/ticdc into checkpoint

7c5fd31

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 18, 2021

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 18, 2021

Ehco1996 mentioned this pull request Nov 18, 2021

dm/loader: support dump tls certs from dm-master #3260

Merged

ti-chi-bot merged commit eb64839 into pingcap:master Nov 18, 2021

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Nov 18, 2021

This is an automated cherry-pick of pingcap#3220

6c2b7d0

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Nov 18, 2021

DM: add checkpoint test for lightning-loader (#3220) #3512

Closed

lance6716 mentioned this pull request Nov 22, 2021

lightning: Add checkpoint test and fix some bug pingcap/dm#2277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM: add checkpoint test for lightning-loader #3220

DM: add checkpoint test for lightning-loader #3220

Little-Wallace commented Nov 1, 2021

ti-chi-bot commented Nov 1, 2021 •

edited

Loading

CLAassistant commented Nov 1, 2021 •

edited

Loading

Ehco1996 Nov 2, 2021

glorv Nov 2, 2021 •

edited

Loading

lance6716 Nov 8, 2021

Ehco1996 commented Nov 4, 2021

lance6716 Nov 4, 2021

Little-Wallace Nov 8, 2021

Ehco1996 commented Nov 8, 2021

lance6716 left a comment •

edited

Loading

lance6716 Nov 8, 2021

lance6716 left a comment

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021 •

edited

Loading

lance6716 commented Nov 18, 2021

lance6716 commented Nov 18, 2021

lance6716 commented Nov 18, 2021

ti-chi-bot commented Nov 18, 2021

lance6716 commented Nov 18, 2021

lance6716 commented Nov 18, 2021

codecov-commenter commented Nov 18, 2021

ti-chi-bot commented Nov 18, 2021

DM: add checkpoint test for lightning-loader #3220

DM: add checkpoint test for lightning-loader #3220

Conversation

Little-Wallace commented Nov 1, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 1, 2021 • edited Loading

CLAassistant commented Nov 1, 2021 • edited Loading

Ehco1996 Nov 2, 2021

Choose a reason for hiding this comment

glorv Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

lance6716 Nov 8, 2021

Choose a reason for hiding this comment

Ehco1996 commented Nov 4, 2021

lance6716 Nov 4, 2021

Choose a reason for hiding this comment

Little-Wallace Nov 8, 2021

Choose a reason for hiding this comment

Ehco1996 commented Nov 8, 2021

lance6716 left a comment • edited Loading

Choose a reason for hiding this comment

lance6716 Nov 8, 2021

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021

lance6716 commented Nov 17, 2021 • edited Loading

lance6716 commented Nov 18, 2021

lance6716 commented Nov 18, 2021

lance6716 commented Nov 18, 2021

ti-chi-bot commented Nov 18, 2021

lance6716 commented Nov 18, 2021

lance6716 commented Nov 18, 2021

codecov-commenter commented Nov 18, 2021

Codecov Report

ti-chi-bot commented Nov 18, 2021

ti-chi-bot commented Nov 1, 2021 •

edited

Loading

CLAassistant commented Nov 1, 2021 •

edited

Loading

glorv Nov 2, 2021 •

edited

Loading

lance6716 left a comment •

edited

Loading

lance6716 commented Nov 17, 2021 •

edited

Loading