lightning: do not output pre-check info when disable check-requirements #27934

Little-Wallace · 2021-09-09T08:30:58Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:
Lightning will check some requirement and it may cause lightning can not run.

What is changed and how it works?

Part of pingcap/tiflow#5833

What's Changed:

do not output pre-check info when disable check-requirements
support SetAppLogger so that DM-worker can use lightning as a library.
return status of lightning so that DM-worker can judge how much data lightning has imported to TiKV.

How it Works:

Check List

Tests

No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

* fix output pre-check info even if user disable check-requirements.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

ti-chi-bot · 2021-09-09T08:30:59Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

3pointer
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.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

kennytm

rest LGTM

kennytm · 2021-09-15T15:54:38Z

br/pkg/lightning/restore/check_info.go

@@ -92,8 +92,7 @@ func (rc *Controller) getClusterAvail(ctx context.Context) (uint64, error) {
 	return clusterAvail, nil
 }

-// ClusterResource check cluster has enough resource to import data. this test can by skipped.
-func (rc *Controller) ClusterResource(ctx context.Context, localSource int64) error {
+func (rc *Controller) clusterResource(ctx context.Context, localSource int64) error {


i think it's useful to keep the doc comment

br/pkg/lightning/lightning.go

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

sleepymole · 2021-09-22T04:44:16Z

br/pkg/lightning/restore/table_restore.go

@@ -314,11 +314,20 @@ func (tr *TableRestore) restoreEngines(pCtx context.Context, rc *Controller, cp
 						dataWorker := rc.closedEngineLimit.Apply()
 						defer rc.closedEngineLimit.Recycle(dataWorker)
 						err = tr.importEngine(ctx, dataClosedEngine, rc, eid, ecp)
+						if rc.status != nil {
+							for _, chunk := range ecp.Chunks {
+								rc.status.FinishedFileSize.Add(chunk.FileMeta.FileSize)


Multiple chunks may share a same file when we split large csv files. The correct value should be chunk.Chunk.EndOffset - chunk.Key.Offset.

But if it started since a checkpoint, we shall calculate the total size rather than how much it does in this job.

sleepymole · 2021-09-22T04:44:25Z

br/pkg/lightning/restore/table_restore.go

 					}
 					if err != nil {
 						setError(err)
 					}
 				}(restoreWorker, engineID, engine)
+			} else {
+				for _, chunk := range engine.Chunks {
+					rc.status.FinishedFileSize.Add(chunk.FileMeta.FileSize)


sleepymole · 2021-09-22T04:55:45Z

br/pkg/lightning/restore/check_template.go

@@ -82,7 +82,7 @@ func (c *SimpleTemplate) Collect(t CheckType, passed bool, msg string) {
 }

 func (c *SimpleTemplate) Success() bool {
-	return c.warnFailedCount+c.criticalFailedCount == 0
+	return c.criticalFailedCount == 0


// If the performance is not as expect or one of critical check not passed. it will stop import task.
Collect(t CheckType, passed bool, msg string)

If c.warnFailedCount > 0, the import task should also be failed. @3pointer PTAL

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Little-Wallace · 2021-09-22T06:24:00Z

/run-check_dev_2

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

3pointer · 2021-09-22T09:44:53Z

br/pkg/lightning/restore/restore.go

-		return errors.Trace(err)
-	}
+	if rc.cfg.App.CheckRequirements {
+		if err := rc.ClusterIsAvailable(ctx); err != nil {


tidb/br/pkg/lightning/restore/check_info.go

Line 171 in 86b464b

if !rc.cfg.App.CheckRequirements {

this line can be removed

3pointer · 2021-09-22T09:45:49Z

br/pkg/lightning/restore/restore.go

 			}
 		}
 	}
-	if rc.tidbGlue.OwnsSQLExecutor() {
+
+	if rc.tidbGlue.OwnsSQLExecutor() && rc.cfg.App.CheckRequirements {
 		// print check info at any time.


3pointer

rest LGTM

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Little-Wallace · 2021-09-22T11:04:29Z

/merge

ti-chi-bot · 2021-09-22T11:04:32Z

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

Commit hash: aac4185

Little-Wallace added 2 commits September 7, 2021 20:10

support dm

09c0086

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

fix requirement

0ffd409

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

add status interface for DM

1f6fdaf

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Little-Wallace mentioned this pull request Sep 15, 2021

Replace Loader with Lightning pingcap/tiflow#5833

Open

5 tasks

fix memory leak

d53df6f

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

kennytm approved these changes Sep 15, 2021

View reviewed changes

ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 15, 2021

Little-Wallace force-pushed the dm branch from b6de768 to 4405ef0 Compare September 22, 2021 03:31

fix comment

4405ef0

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

sleepymole reviewed Sep 22, 2021

View reviewed changes

Little-Wallace added 2 commits September 22, 2021 13:57

fix report

35fc4e4

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Merge branch 'master' into dm

c350c86

fix test

86b464b

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

3pointer reviewed Sep 22, 2021

View reviewed changes

address comment

dc49476

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

3pointer approved these changes Sep 22, 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 Sep 22, 2021

Merge branch 'master' into dm

aac4185

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

ti-chi-bot merged commit 1e8b933 into pingcap:master Sep 22, 2021

kennytm mentioned this pull request Sep 24, 2021

lightning: "panic: interface conversion: kv.Rows is tidb.tidbRows, not *kv.KvPairs" #28276

Closed

3pointer mentioned this pull request Oct 25, 2021

br/lightning: fix the integration test cases #29062

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: do not output pre-check info when disable check-requirements #27934

lightning: do not output pre-check info when disable check-requirements #27934

Little-Wallace commented Sep 9, 2021 •

edited

Loading

ti-chi-bot commented Sep 9, 2021 •

edited

Loading

kennytm left a comment

kennytm Sep 15, 2021

Little-Wallace Sep 18, 2021

sleepymole Sep 22, 2021 •

edited

Loading

Little-Wallace Sep 22, 2021

sleepymole Sep 22, 2021

sleepymole Sep 22, 2021

Little-Wallace commented Sep 22, 2021

3pointer Sep 22, 2021

3pointer Sep 22, 2021

3pointer left a comment

Little-Wallace commented Sep 22, 2021

ti-chi-bot commented Sep 22, 2021

lightning: do not output pre-check info when disable check-requirements #27934

lightning: do not output pre-check info when disable check-requirements #27934

Conversation

Little-Wallace commented Sep 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 Sep 9, 2021 • edited Loading

kennytm left a comment

Choose a reason for hiding this comment

kennytm Sep 15, 2021

Choose a reason for hiding this comment

Little-Wallace Sep 18, 2021

Choose a reason for hiding this comment

sleepymole Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Little-Wallace Sep 22, 2021

Choose a reason for hiding this comment

sleepymole Sep 22, 2021

Choose a reason for hiding this comment

sleepymole Sep 22, 2021

Choose a reason for hiding this comment

Little-Wallace commented Sep 22, 2021

3pointer Sep 22, 2021

Choose a reason for hiding this comment

3pointer Sep 22, 2021

Choose a reason for hiding this comment

3pointer left a comment

Choose a reason for hiding this comment

Little-Wallace commented Sep 22, 2021

ti-chi-bot commented Sep 22, 2021

Little-Wallace commented Sep 9, 2021 •

edited

Loading

ti-chi-bot commented Sep 9, 2021 •

edited

Loading

sleepymole Sep 22, 2021 •

edited

Loading