checker(dm): port some precheck from lightning #7617

lance6716 · 2022-11-15T10:41:14Z

What problem does this PR solve?

Issue Number: ref #3510

What is changed and how it works?

port some simple precheck from lightning

Check List

Tests

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

build a v7.0.0 TiDB

command: start-task /tmp/dm_test/lightning_mode/dm-task.yaml --remove-meta "result": true count: 0 != expected: 3
{
    "result": false,
    "msg": "",
    "sources": [
    ],
    "checkResult": "[code=26005:class=dm-master:scope=internal:level=medium], Message: fail to check synchronization configuration with type: check was failed, please see detail
        detail: {
                "results": [
                        {
                                "id": 12,
                                "name": "mysql_version",
                                "desc": "check whether mysql version is satisfied",
                                "state": "warn",
                                "errors": [
                                        {
                                                "severity": "warn",
                                                "short_error": "version suggested less than 8.0.0 but got 8.0.21"
                                        }
                                ],
                                "extra": "address of db instance - 127.0.0.1:3307"
                        },
                        {
                                "id": 24,
                                "name": "lightning_cluster_version",
                                "desc": "check downstream cluster version",
                                "state": "fail",
                                "errors": [
                                        {
                                                "severity": "fail",
                                                "short_error": "Cluster version check failed: [Lightning:Common:ErrVersionMismatch]TiDB version too new, expected to be within [4.0.0, 7.0.0), found '7.0.0'"
                                        }
                                ]
                        }
                ],
                "summary": {
                        "passed": false,
                        "total": 25,
                        "successful": 23,
                        "failed": 1,
                        "warning": 1
                }
        }"
}

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-11-15T10:41:15Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

GMHDBJD
buchuitoudegou

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.

lance6716 · 2022-11-15T10:41:52Z

#5289 need to fix this

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-23T04:18:16Z

ptal @gozssky @lichunzhu

codecov-commenter · 2022-11-23T04:26:23Z

Codecov Report

Merging #7617 (0d952fa) into master (6d24a57) will decrease coverage by 0.0114%.
The diff coverage is 46.3576%.

Additional details and impacted files

Flag	Coverage Δ
cdc	`66.2698% <5.8823%> (-0.0321%)`	⬇️
dm	`52.1924% <51.4925%> (+0.0072%)`	⬆️
engine	`64.0284% <ø> (+0.0316%)`	⬆️

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

@@               Coverage Diff                @@
##             master      #7617        +/-   ##
================================================
- Coverage   59.9034%   59.8920%   -0.0115%     
================================================
  Files           810        811         +1     
  Lines         93255      93343        +88     
================================================
+ Hits          55863      55905        +42     
- Misses        32535      32580        +45     
- Partials       4857       4858         +1

buchuitoudegou

rest lgtm

buchuitoudegou · 2022-11-23T09:04:29Z

dm/checker/checker.go

@@ -330,6 +332,45 @@ func (c *Checker) Init(ctx context.Context) (err error) {
 		}
 	}

+	if instance.cfg.Mode != config.ModeIncrement && instance.cfg.LoaderConfig.ImportMode == config.LoadModePhysical {


Do we need some checks for tidb backend?

the checks in this PR is only for local backend because they are related to storage layer. I'll add more later

Does DM support local backend now? 🤔

buchuitoudegou · 2022-11-23T09:06:24Z

dm/pkg/checker/lightning_test.go

+	msg  string
+}
+
+func (m mockPrecheckItem) Check(ctx context.Context) (*restore.CheckResult, error) {


I suppose we should add some integratoin test too? The unit test seems only mock the return result.

It's hard to generate such test case in the real cluster, I have a manually test with self-built TiDB in PR description

…-precheck Signed-off-by: lance6716 <lance6716@gmail.com>

lichunzhu

Rest LGTM. It sames that we will do all the lightning prechecks now?

dm/checker/checker.go

Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>

lance6716 · 2022-11-23T13:02:09Z

Rest LGTM. It sames that we will do all the lightning prechecks now?

There're more checks that need some adjustment to migrated into lightning in DM. For example, checking downstream has enough free space with dump files should be changed, because dump files are generated intermediately in the task.

GMHDBJD

LGTM

buchuitoudegou · 2022-11-24T06:15:45Z

dm/config/checking_item.go

+	// lightning prechecks
+	LightningEmptyRegionChecking:        "lightning empty region checking item",
+	LightningRegionDistributionChecking: "lightning region distribution checking item",
+	LightningDownstreamVersionChecking:  "lightning downstream version checking item",


I remember we need to check the empty space for local backend. Is it necessary here?

lance6716 · 2022-11-24T06:24:32Z

/merge

ti-chi-bot · 2022-11-24T06:24:35Z

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

Commit hash: a609857

D3Hunter

rest lgtm

D3Hunter · 2022-11-24T06:21:48Z

dm/checker/checker.go

+		if err != nil {
+			return err
+		}
+		// bypass an Adjust error


what error?

updated

// Adjust will raise error when this field is empty, so we set any non empty value here.

D3Hunter · 2022-11-24T06:25:43Z

dm/pkg/checker/lightning.go

+
+// Name implements the RealChecker interface.
+func (c *LightningEmptyRegionChecker) Name() string {
+	return "lightning_empty_region"


why not use config.LightningEmptyRegionChecking

other checkers do not have same name with checking items. I'll ask PM

lance6716 · 2022-11-24T06:36:40Z

/hold

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-24T08:48:00Z

/unhold

/merge

ti-chi-bot · 2022-11-24T08:48:04Z

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

Commit hash: 15dc4d4

lance6716 · 2022-11-24T10:24:43Z

/run-all-tests

lance6716 · 2022-11-24T10:25:06Z

/check-issue-triage-complete

--wip-- [skip ci]

5f98a30

lance6716 added 2 commits November 17, 2022 09:59

save work

06dbbaa

Signed-off-by: lance6716 <lance6716@gmail.com>

finish

ce9fd63

Signed-off-by: lance6716 <lance6716@gmail.com>

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. do-not-merge/needs-linked-issue labels Nov 17, 2022

lance6716 changed the title ~~[WIP]checker(dm): port some precheck from lightning~~ checker(dm): port some precheck from lightning Nov 17, 2022

lance6716 requested review from sleepymole and lichunzhu November 17, 2022 06:20

lance6716 added the area/dm Issues or PRs related to DM. label Nov 17, 2022

lance6716 added 3 commits November 17, 2022 15:41

fix fmt

1061df5

Signed-off-by: lance6716 <lance6716@gmail.com>

fix lint

fdc4793

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'master' into add-lightning-precheck

6230a88

buchuitoudegou reviewed Nov 23, 2022

View reviewed changes

Merge branch 'master' of github.com:pingcap/tiflow into add-lightning…

eb6a184

…-precheck Signed-off-by: lance6716 <lance6716@gmail.com>

lichunzhu reviewed Nov 23, 2022

View reviewed changes

dm/checker/checker.go Outdated Show resolved Hide resolved

Update dm/checker/checker.go

a609857

Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>

GMHDBJD approved these changes Nov 24, 2022

View reviewed changes

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

buchuitoudegou approved these changes Nov 24, 2022

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 24, 2022

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

Merge branch 'master' into add-lightning-precheck

0d952fa

D3Hunter approved these changes Nov 24, 2022

View reviewed changes

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

change some message

15dc4d4

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 24, 2022

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 24, 2022

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

ti-chi-bot added 2 commits November 24, 2022 16:48

Merge branch 'master' into add-lightning-precheck

0b765db

Merge branch 'master' into add-lightning-precheck

7db68c7

ti-chi-bot merged commit e0bce5e into pingcap:master Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checker(dm): port some precheck from lightning #7617

checker(dm): port some precheck from lightning #7617

lance6716 commented Nov 15, 2022 •

edited

Loading

ti-chi-bot commented Nov 15, 2022 •

edited

Loading

lance6716 commented Nov 15, 2022

lance6716 commented Nov 23, 2022

codecov-commenter commented Nov 23, 2022 •

edited

Loading

buchuitoudegou left a comment

buchuitoudegou Nov 23, 2022

lance6716 Nov 23, 2022 •

edited

Loading

sleepymole Nov 24, 2022

lance6716 Nov 24, 2022

buchuitoudegou Nov 23, 2022

lance6716 Nov 23, 2022

lichunzhu left a comment

lance6716 commented Nov 23, 2022

GMHDBJD left a comment

buchuitoudegou Nov 24, 2022

lance6716 commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

D3Hunter left a comment

D3Hunter Nov 24, 2022

lance6716 Nov 24, 2022

D3Hunter Nov 24, 2022

lance6716 Nov 24, 2022

lance6716 commented Nov 24, 2022

lance6716 commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

lance6716 commented Nov 24, 2022

lance6716 commented Nov 24, 2022

checker(dm): port some precheck from lightning #7617

checker(dm): port some precheck from lightning #7617

Conversation

lance6716 commented Nov 15, 2022 • edited Loading

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 Nov 15, 2022 • edited Loading

lance6716 commented Nov 15, 2022

lance6716 commented Nov 23, 2022

codecov-commenter commented Nov 23, 2022 • edited Loading

Codecov Report

buchuitoudegou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 Nov 23, 2022 • edited Loading

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

lichunzhu left a comment

Choose a reason for hiding this comment

lance6716 commented Nov 23, 2022

GMHDBJD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

D3Hunter 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

lance6716 commented Nov 24, 2022

lance6716 commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

lance6716 commented Nov 24, 2022

lance6716 commented Nov 24, 2022

lance6716 commented Nov 15, 2022 •

edited

Loading

ti-chi-bot commented Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 23, 2022 •

edited

Loading

lance6716 Nov 23, 2022 •

edited

Loading