loader(dm): simplify lightning checkpoint and add clean meta #3813

glorv · 2021-12-09T11:45:29Z

What problem does this PR solve?

Support remove lighting checkpoint meta when run start-task --remove-meta, https://github.com/pingcap/ticdc/issues/3510

Close #3509

What is changed and how it works?

Simplify lightning checkpoint logic by:
- change lightning checkpoint type from mysql to file and stop the file checkpoint at dump dir
- Change lightning_checkpoint_list table in downstream for (task-name, worker-name) to (task-name, source-name, status). Lightning depend this table to check its load status (fresh, loading, finished).

Check List

Tests

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

Code changes

Has persistent data change

Side effects

Breaking backward compatibility
- This new implement is not compatible with the previous implement because lightning checkpoint data is changed.

Related changes

Release note

None

ti-chi-bot · 2021-12-09T11:45:31Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996
lichunzhu

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.

glorv · 2021-12-09T12:26:20Z

/run-dm-integration-test

glorv · 2021-12-09T12:33:39Z

/run-dm-integration-test

glorv · 2021-12-10T02:50:40Z

/run-dm-integration-test

glorv · 2021-12-10T06:18:44Z

/run-dm-integration-test

glorv · 2021-12-10T08:22:56Z

/run-dm-integration-test

glorv · 2021-12-10T09:39:05Z

/run-verify

dm/loader/checkpoint.go

dm/loader/lightning.go

dm/loader/checkpoint.go

Ehco1996 · 2021-12-13T00:44:17Z

dm/loader/checkpoint.go

+
+const (
+	lightningStatusInit lightingLoadStatus = iota
+	lightningStatusRunning


maybe reuse CheckpointStatus in tidb/pkg/checkpoints ?

do you mean the real tidb-lightning's CheckpointStatus ? I think because there are so many state in it, it maybe hard to understand of not familiar with tidb-lightning

oh my fault i got the two status mixed up, now i think the current implementaion make more sense

btw, how about direct use char like init,running , finished in db? maybe this can increase readability

Ehco1996 · 2021-12-15T01:53:22Z

dm/loader/checkpoint.go

+
+const (
+	lightningStatusInit lightingLoadStatus = iota
+	lightningStatusRunning


oh my fault i got the two status mixed up, now i think the current implementaion make more sense

btw, how about direct use char like init,running , finished in db? maybe this can increase readability

Ehco1996 · 2021-12-15T02:02:13Z

dm/loader/checkpoint.go

+	return lightningStatusInit, nil
+}
+
+func (cp *LightningCheckpointList) RemoveTaskCheckPoint(ctx context.Context) error {


when is this function to be called ?

No used now. It is orginally designed for being calling at cleanup meta, but the current design just call raw drop table like other tables. 😅

so we can delete this func?

Ehco1996

restLGTM

Ehco1996 · 2021-12-16T00:34:28Z

dm/loader/checkpoint.go

+	return lightningStatusInit, nil
+}
+
+func (cp *LightningCheckpointList) RemoveTaskCheckPoint(ctx context.Context) error {


so we can delete this func?

lichunzhu

Should lightning's pb checkpoint be removed when we remove-meta? Or maybe left a comment?
We don't have a task contains only load+sync, but I'm not sure whether we will have this kind of task in the future or not.

glorv · 2021-12-17T02:46:44Z

Should lightning's pb checkpoint be removed when we remove-meta? Or maybe left a comment? We don't have a task contains only load+sync, but I'm not sure whether we will have this kind of task in the future or not.

The checkpoint is under dump directory, so it can be cleaned up when dump data is cleaned.

codecov-commenter · 2021-12-17T05:00:52Z

Codecov Report

Merging #3813 (5c489fd) into master (3873d39) will decrease coverage by 1.7643%.
The diff coverage is 61.7801%.

Flag	Coverage Δ
cdc	`58.1969% <61.7801%> (-0.0396%)`	⬇️
dm	`52.8655% <ø> (-3.1690%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #3813        +/-   ##
================================================
- Coverage   57.0741%   55.3098%   -1.7644%     
================================================
  Files           478        484         +6     
  Lines         56551      59060      +2509     
================================================
+ Hits          32276      32666       +390     
- Misses        20978      23090      +2112     
- Partials       3297       3304         +7

glorv · 2021-12-21T09:29:55Z

/merge

ti-chi-bot · 2021-12-21T09:29:57Z

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

Commit hash: 5c489fd

ti-chi-bot · 2021-12-22T00:39:47Z

@glorv: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

Ehco1996 · 2021-12-22T00:40:23Z

/run-kafka-integration-test /tidb=pr/30773
/run-integration-tests /tidb=pr/30773

…#3813)

simplify lightning checkpoint and add clean meta

86a31e7

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

fix lint

f849a07

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 9, 2021

fix checkpoint dsn

8201a52

fix unit test

902dca6

glorv marked this pull request as ready for review December 10, 2021 06:24

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

glorv added 2 commits December 10, 2021 16:22

add unit test

ea02fbc

Merge branch 'master' into lighting-cp

62a73ca

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 10, 2021

fix lint

7ee0078

glorv requested review from lance6716 and Little-Wallace December 10, 2021 09:26

Ehco1996 reviewed Dec 13, 2021

View reviewed changes

glorv added 2 commits December 14, 2021 19:52

Merge branch 'master' into lighting-cp

adf8a7b

resolve comments

44f90ee

Ehco1996 reviewed Dec 15, 2021

View reviewed changes

glorv added 4 commits December 15, 2021 13:53

Merge branch 'master' of ssh://github.com/pingcap/ticdc into lighting-cp

174efba

change lightning status to varchar in checkpoint table

ec90513

remove useless import

f2ea0cd

fix lint

83156f2

Ehco1996 reviewed Dec 16, 2021

View reviewed changes

lichunzhu approved these changes Dec 16, 2021

View reviewed changes

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

remove unused method

09df71e

Ehco1996 approved these changes Dec 17, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 17, 2021

Merge branch 'master' of ssh://github.com/pingcap/ticdc into lighting-cp

5c489fd

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2021

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

ti-chi-bot added 3 commits December 21, 2021 17:30

Merge branch 'master' into lighting-cp

cbf0641

Merge branch 'master' into lighting-cp

020d020

Merge branch 'master' into lighting-cp

8224103

ti-chi-bot merged commit d84f15b into pingcap:master Dec 22, 2021

glorv deleted the lighting-cp branch December 28, 2021 11:44

zhaoxinyu pushed a commit to zhaoxinyu/ticdc that referenced this pull request Dec 29, 2021

loader(dm): simplify lightning checkpoint and add clean meta (pingcap…

3de5122

…#3813)

lance6716 mentioned this pull request Mar 22, 2022

lightning: support inject external storage when as library pingcap/tidb#33303

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loader(dm): simplify lightning checkpoint and add clean meta #3813

loader(dm): simplify lightning checkpoint and add clean meta #3813

glorv commented Dec 9, 2021 •

edited

Loading

ti-chi-bot commented Dec 9, 2021 •

edited

Loading

glorv commented Dec 9, 2021

glorv commented Dec 9, 2021

glorv commented Dec 10, 2021

glorv commented Dec 10, 2021

glorv commented Dec 10, 2021

glorv commented Dec 10, 2021

Ehco1996 Dec 13, 2021

glorv Dec 14, 2021

Ehco1996 Dec 15, 2021

glorv Dec 15, 2021

Ehco1996 Dec 15, 2021

Ehco1996 Dec 15, 2021

glorv Dec 15, 2021 •

edited

Loading

Ehco1996 Dec 16, 2021

Ehco1996 left a comment

Ehco1996 Dec 16, 2021

lichunzhu left a comment

glorv commented Dec 17, 2021

codecov-commenter commented Dec 17, 2021 •

edited

Loading

glorv commented Dec 21, 2021

ti-chi-bot commented Dec 21, 2021

ti-chi-bot commented Dec 22, 2021

Ehco1996 commented Dec 22, 2021

loader(dm): simplify lightning checkpoint and add clean meta #3813

loader(dm): simplify lightning checkpoint and add clean meta #3813

Conversation

glorv commented Dec 9, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 9, 2021 • edited Loading

glorv commented Dec 9, 2021

glorv commented Dec 9, 2021

glorv commented Dec 10, 2021

glorv commented Dec 10, 2021

glorv commented Dec 10, 2021

glorv commented Dec 10, 2021

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

glorv Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu left a comment

Choose a reason for hiding this comment

glorv commented Dec 17, 2021

codecov-commenter commented Dec 17, 2021 • edited Loading

Codecov Report

glorv commented Dec 21, 2021

ti-chi-bot commented Dec 21, 2021

ti-chi-bot commented Dec 22, 2021

Ehco1996 commented Dec 22, 2021

glorv commented Dec 9, 2021 •

edited

Loading

ti-chi-bot commented Dec 9, 2021 •

edited

Loading

glorv Dec 15, 2021 •

edited

Loading

codecov-commenter commented Dec 17, 2021 •

edited

Loading