Lightning: add a pre check before import starts. #999

3pointer · 2021-04-13T05:26:24Z

What problem does this PR solve?

Add some checks before lightning start imports.

close #1107

What is changed and how it works?

Replace checkRequirements with PreCheck.
Add DataCheck after restoreSchemas
All checks add in check_info.go
Use go-pretty to render output template.
Depreacated skip-chech-path

Check List

Tests

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

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release Note

Add pre-check before lightning starts.

3pointer · 2021-04-20T11:18:40Z

/rebuild

pkg/lightning/common/util.go

glorv · 2021-04-21T06:18:30Z

pkg/lightning/config/config.go

+	CaseSensitive    bool                `toml:"case-sensitive" json:"case-sensitive"`
+	StrictFormat     bool                `toml:"strict-format" json:"strict-format"`
+	DefaultFileRules bool                `toml:"default-file-rules" json:"default-file-rules"`
+	IgnoreColumns    map[string][]string `toml:"ignore-columns" json:"ignore-columns"`


should use []IgnoreColumns here?

no, we need specify db.table for this config.

I rethinked about this , I think it's better to use the same rule as table filter to support regexp

pkg/lightning/config/config.go

pkg/storage/s3.go

3pointer · 2021-05-07T12:17:09Z

/run-integration-tests

3pointer · 2021-05-13T03:33:56Z

/run-integration-tests

3pointer · 2021-05-24T09:19:34Z

/run-all-tests

CLAassistant · 2021-05-25T08:04:16Z

All committers have signed the CLA.

pkg/lightning/config/config.go

pkg/lightning/mydump/loader.go

pkg/lightning/restore/check_info.go

pkg/lightning/restore/restore.go

pkg/lightning/restore/restore_test.go

pkg/task/common.go

pkg/lightning/restore/restore_test.go

pkg/lightning/config/config.go

glorv · 2021-06-02T03:29:55Z

@3pointer Please rebase your pr to make the cla check happy

kennytm · 2021-06-04T05:29:32Z

@3pointer the commit 85440dd's author is still root <root>.

kennytm · 2021-06-04T07:42:29Z

pkg/lightning/common/util.go

+func AskForConfirmation() bool {
+	var response string
+	fmt.Println("please type [y/N] to continue:")
+	_, err := fmt.Scanln(&response)


have i missed something? i've never seen any design doc saying that supporting pre-check will turn Lightning into an interactive-CLI tool

looking at the changes to the test cases show that introducing interactivity will break a lot of Lightning use cases. also given the history of dmctl I seriously doubt introducing interactivity is a good idea.

I add the interactivity because I found lightning will start to import automatically even we had some warning in pre-check.
so the user had to kill lightning and start it again, and if the user(it was me) didn't fix the waring in one time. he must kill lightning and start again and again.

we could make the checks fatal, and having --disable-checks=aaa,bbb,ccc (or the opposite --enable-checks=xxx,yyy,zzz) to toggle the checks. the error message to mention the corresponding CLI paramters and config.

Ok, I made the checks fatal and reuse --check-requirements to disable the checks.
Because it's not necessary to skip some checks.

glorv · 2021-06-05T15:37:29Z

pkg/lightning/config/config.go

+	CaseSensitive    bool                `toml:"case-sensitive" json:"case-sensitive"`
+	StrictFormat     bool                `toml:"strict-format" json:"strict-format"`
+	DefaultFileRules bool                `toml:"default-file-rules" json:"default-file-rules"`
+	IgnoreColumns    map[string][]string `toml:"ignore-columns" json:"ignore-columns"`


I rethinked about this , I think it's better to use the same rule as table filter to support regexp

glorv · 2021-06-05T15:39:15Z

pkg/lightning/restore/check_info.go

+	pdStores    = "/pd/api/v1/stores"
+	pdReplicate = "/pd/api/v1/config/replicate"
+
+	defaultCSVSize = 10 * units.GiB


does 1GiB already enough? I think 10GB is too large

pkg/lightning/restore/check_info.go

pkg/lightning/restore/restore_test.go

pkg/lightning/restore/restore.go

pkg/lightning/restore/check_info.go

glorv · 2021-06-08T06:09:04Z

pkg/lightning/mydump/loader.go

-			return errors.New("no schema create sql files found. Please either set `mydumper.no-schema` to true or add schema sql file for each database")
-		}
+	// setup database schema
+	if len(s.dbSchemas) != 0 {


Suggested change

if len(s.dbSchemas) != 0 {

if len(s.dbSchemas) != 0 || len(s.tableSchemas) != 0 {

glorv

LGTM

ti-chi-bot · 2021-06-09T18:25:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

glorv
kennytm

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.

3pointer · 2021-06-10T02:01:04Z

/merge

ti-chi-bot · 2021-06-10T02:01:06Z

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

Commit hash: 2a5564b

ti-chi-bot · 2021-06-10T02:03:06Z

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

ti-chi-bot added do-not-merge/work-in-progress status/WIP size/XXL labels Apr 13, 2021

ti-chi-bot requested review from glorv and overvenus April 13, 2021 05:26

3pointer force-pushed the lightning_pre_check branch from 9e02ac5 to c16890a Compare April 20, 2021 10:03

glorv reviewed Apr 21, 2021

View reviewed changes

pkg/lightning/common/util.go Outdated Show resolved Hide resolved

glorv reviewed Apr 21, 2021

View reviewed changes

3pointer force-pushed the lightning_pre_check branch 2 times, most recently from e634937 to 7a7abad Compare April 27, 2021 06:55

3pointer force-pushed the lightning_pre_check branch from 1983aa9 to a4cf617 Compare May 12, 2021 12:28

glorv added the component/import label May 13, 2021

3pointer marked this pull request as ready for review May 25, 2021 09:37

ti-chi-bot removed the do-not-merge/work-in-progress status/WIP label May 25, 2021

glorv added this to the v5.1.0 milestone Jun 2, 2021

glorv reviewed Jun 2, 2021

View reviewed changes

3pointer force-pushed the lightning_pre_check branch from 7b2dcc0 to 8ccce25 Compare June 3, 2021 12:38

kennytm reviewed Jun 4, 2021

View reviewed changes

3pointer force-pushed the lightning_pre_check branch 3 times, most recently from 9700be9 to 75d3607 Compare June 4, 2021 11:40

ti-chi-bot added the needs-rebase label Jun 4, 2021

3pointer force-pushed the lightning_pre_check branch 2 times, most recently from 9700be9 to 6b3a8c2 Compare June 4, 2021 12:09

3pointer force-pushed the lightning_pre_check branch from 6b3a8c2 to e7f1ed5 Compare June 5, 2021 00:02

ti-chi-bot added the needs-rebase label Jun 5, 2021

3pointer force-pushed the lightning_pre_check branch from e7f1ed5 to d14e527 Compare June 5, 2021 00:10

3pointer added 2 commits June 5, 2021 08:12

lightning: add pre check

1458172

fix conflicts

95a5b3a

3pointer force-pushed the lightning_pre_check branch from d14e527 to 95a5b3a Compare June 5, 2021 00:14

ti-chi-bot removed the needs-rebase label Jun 5, 2021

glorv reviewed Jun 5, 2021

View reviewed changes

3pointer added 5 commits June 7, 2021 20:23

address comments

36d1f43

fix build

d9b7def

address comments

b56a34c

fix build

f53ef84

fix test

3f3cd62

glorv reviewed Jun 8, 2021

View reviewed changes

address comments

2a5564b

3pointer added the needs-cherry-pick-release-5.1 label Jun 9, 2021

glorv approved these changes Jun 9, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 LGTM1 label Jun 9, 2021

kennytm approved these changes Jun 9, 2021

View reviewed changes

ti-chi-bot added status/LGT2 LGTM2 and removed status/LGT1 LGTM1 labels Jun 9, 2021

ti-chi-bot added the status/can-merge label Jun 10, 2021

ti-chi-bot merged commit 919e650 into pingcap:master Jun 10, 2021

ti-chi-bot mentioned this pull request Jun 10, 2021

Lightning: add a pre check before import starts. (#999) #1192

Merged

ti-chi-bot added a commit that referenced this pull request Jun 10, 2021

Lightning: add a pre check before import starts. (#999) (#1192)

bd77eff

dveeden mentioned this pull request Jul 29, 2021

confusing "is a directory" error when tidb-lightning can't find table schema using no-schema #1394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lightning: add a pre check before import starts. #999

Lightning: add a pre check before import starts. #999

3pointer commented Apr 13, 2021 •

edited by glorv

Loading

3pointer commented Apr 20, 2021

glorv Apr 21, 2021

3pointer Jun 3, 2021

glorv Jun 5, 2021

3pointer commented May 7, 2021

3pointer commented May 13, 2021

3pointer commented May 24, 2021

CLAassistant commented May 25, 2021 •

edited

Loading

glorv commented Jun 2, 2021

kennytm commented Jun 4, 2021

kennytm Jun 4, 2021 •

edited

Loading

glorv Jun 4, 2021

3pointer Jun 4, 2021

kennytm Jun 5, 2021

3pointer Jun 7, 2021

glorv Jun 5, 2021

glorv Jun 5, 2021

glorv Jun 8, 2021

glorv left a comment

ti-chi-bot commented Jun 9, 2021

3pointer commented Jun 10, 2021

ti-chi-bot commented Jun 10, 2021

ti-chi-bot commented Jun 10, 2021

	if len(s.dbSchemas) != 0 {
	if len(s.dbSchemas) != 0 \|\| len(s.tableSchemas) != 0 {

Lightning: add a pre check before import starts. #999

Lightning: add a pre check before import starts. #999

Conversation

3pointer commented Apr 13, 2021 • edited by glorv Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release Note

3pointer commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3pointer commented May 7, 2021

3pointer commented May 13, 2021

3pointer commented May 24, 2021

CLAassistant commented May 25, 2021 • edited Loading

glorv commented Jun 2, 2021

kennytm commented Jun 4, 2021

kennytm Jun 4, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glorv left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jun 9, 2021

3pointer commented Jun 10, 2021

ti-chi-bot commented Jun 10, 2021

ti-chi-bot commented Jun 10, 2021

3pointer commented Apr 13, 2021 •

edited by glorv

Loading

CLAassistant commented May 25, 2021 •

edited

Loading

kennytm Jun 4, 2021 •

edited

Loading