-
Notifications
You must be signed in to change notification settings - Fork 283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
checker(dm): port some precheck from lightning #7617
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
#5289 need to fix this |
ptal @gozssky @lichunzhu |
Codecov Report
Additional details and impacted files
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need some checks for tidb backend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the checks in this PR is only for local backend because they are related to storage layer. I'll add more later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does DM support local backend now? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msg string | ||
} | ||
|
||
func (m mockPrecheckItem) Check(ctx context.Context) (*restore.CheckResult, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we should add some integratoin test too? The unit test seems only mock the return result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM. It sames that we will do all the lightning prechecks now?
Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
dm/config/checking_item.go
Outdated
// lightning prechecks | ||
LightningEmptyRegionChecking: "lightning empty region checking item", | ||
LightningRegionDistributionChecking: "lightning region distribution checking item", | ||
LightningDownstreamVersionChecking: "lightning downstream version checking item", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we need to check the empty space for local backend. Is it necessary here?
/merge |
This pull request has been accepted and is ready to merge. Commit hash: a609857
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
dm/checker/checker.go
Outdated
if err != nil { | ||
return err | ||
} | ||
// bypass an Adjust error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
// Adjust will raise error when this field is empty, so we set any non empty value here.
|
||
// Name implements the RealChecker interface. | ||
func (c *LightningEmptyRegionChecker) Name() string { | ||
return "lightning_empty_region" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use config.LightningEmptyRegionChecking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other checkers do not have same name with checking items. I'll ask PM
/hold |
Signed-off-by: lance6716 <lance6716@gmail.com>
/unhold /merge |
This pull request has been accepted and is ready to merge. Commit hash: 15dc4d4
|
/run-all-tests |
/check-issue-triage-complete |
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
build a v7.0.0 TiDB
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note