lightning: support inject external storage when as library #33303

lance6716 · 2022-03-22T03:56:02Z

What problem does this PR solve?

Issue Number: ref #33281

Problem Summary:

What is changed and how it works?

as title

Check List

Tests

Integration test

Side effects

Documentation

Release note

None

ti-chi-bot · 2022-03-22T03:56:04Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
gozssky

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.

sre-bot · 2022-03-22T04:06:51Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/5ac26f09ba5888a07b2345d12451172bc8a5c6b5

Signed-off-by: lance6716 <[email protected]>

sleepymole · 2022-03-22T06:14:56Z

br/pkg/lightning/run_options.go

+type options struct {
+	glue               glue.Glue
+	externalStorage    storage.ExternalStorage
+	cpNameInExtStorage string


Maybe just checkpointName is better?

sleepymole · 2022-03-22T07:00:33Z

br/pkg/lightning/lightning.go

+		return common.NormalizeError(errors.New("WithExternalStorage and WithGlue can't be both set"))
+	}
+	if o.cpNameInExtStorage != "" && o.glue != nil {
+		return common.NormalizeError(errors.New("WithCpNameInExtStorage and WithGlue can't be both set"))


common.ErrInvalidArgument? It's not meaningful to call NormalizeError for an explicit error.

sleepymole · 2022-03-22T07:15:40Z

br/pkg/lightning/restore/restore.go

+	// lightning via SQL will implement its glue, to let lightning use host TiDB's environment
+	Glue glue.Glue
+	// when not OwnExtStorage, checkpoint can also be saved in framework-created ExtStorage by setting this field
+	CpNameInExtStorage string


Lightning assumes all files in external storage are source data. I think it's not a good way to put checkpoint file in it, although we can use file route to ignore specific file. I suggest using a separate external storage to store checkpoint. If you really want to use the same external storage, you can pass the same object.

This is already the behaviour of lightning in DM pingcap/tiflow#3813

How does DM works without this PR before?

you can search https://github.com/pingcap/tiflow/pull/3813/files

https://github.com/pingcap/tiflow/pull/3813/files#diff-dc63bb506f94f15965a3d2fed1dcf08ced7e068fb8b07488f90895dfd40e0e38R196

cfg.Checkpoint.Driver = lcfg.CheckpointDriverFile cpPath := filepath.Join(l.cfg.LoaderConfig.Dir, lightningCheckpointFileName) cfg.Checkpoint.DSN = cpPath

It seems it just set checkpoint path inside the source dir, but it doesn't limit it setting to other path.

Using the external storage for source data and checkpoint has potential risks that the checkpoint file may be treated as a source file to import. So I think we shouldn't put checkpoint in the same external storage. But if DM still want to put them together, two external storage can be created. e.g source dir: file:///dump checkpoint: file:///dump/checkpoint.

I think most of concerns are about DM/dataflow engine should be more careful to build a config/environment for lightning when storing file checkpoint in dump folder, not related to lightning's inner logic. For example,

the checkpoint file may be treated as a source file to import.

In DM or dataflow engine's case, dumpling is used to create dump files, so we may limit the dump file matching pattern with this apriori knowledge to avoid bugs.

But if we actually store them at two place, (which requires two different arguments for two storage parameters), lightning has to handle the case that only one of them exists. And risk is higher since it's rely on two storages are available rather than one.

lightning has to handle the case that only one of them exists.

Maybe we can let the two storages required and disallow one of them is nil. Or create checkpointsDB out of NewRestoreController and pass checkpointsDB to it.

lightning has to handle the case that only one of them exists.

Maybe we can let the two storages required and disallow one of them is nil. Or create checkpointsDB out of NewRestoreController and pass checkpointsDB to it.

Oh I mean files on one of two storages are cleaned. Can lightning report error when only checkpoint exists? And lost checkpoint will cause some import restarted, which will not happen if we store them together.

Maybe not. I think these are two concerns.

For Lightning, I don't hope checkpoint can only store in dump dir's external storage.

For DM, If you want to avoid "one of them loss", you can pass the same external storage object to both dump dir and checkpoint path.

OK, I'll add two storage parameters. Lightning as binary can still use old config Checkpoint.DSN to specify the file checkpoint location.

Signed-off-by: lance6716 <[email protected]>

WizardXiao

I have no other divergence besides this checkpoint divergence, although dm use the same dir to save chepoint and data files.
how about checkpoint will use self storage only when there has a dsn configuration?

lance6716 · 2022-03-22T09:45:51Z

I have no other divergence besides this checkpoint divergence, although dm use the same dir to save chepoint and data files. how about checkpoint will use self storage only when there has a dsn configuration?

For dataflow engine's case, it will not expose S3 URI to lightning, so this will not happen. For DM's case the logic is unchanged as before.

…orage

Signed-off-by: lance6716 <[email protected]>

sleepymole

rest LGTM

sleepymole · 2022-03-22T10:24:38Z

br/pkg/lightning/lightning.go

+	}
+	if o.checkpointStorage != nil && o.glue != nil {
+		return common.ErrInvalidArgument.GenWithStack("WithCheckpointStorage and WithGlue can't be both set")
+	}


Could you please explain why storage and glue can't be both set?

Signed-off-by: lance6716 <[email protected]>

sleepymole · 2022-03-23T02:36:28Z

br/pkg/lightning/lightning.go

+
+	// pre-check about options
+	// glue should be set when lightning in TiDB, and storages should be set when lightning in DM/dataflow engine,
+	// so they should not both be set.


It seems glue and storage can both set technically. If we implement Lightning on SQL, TiDB also use Lightning as a library. At that time, TiDB may both set glue and storage.

Besides, Lightning is an independent component or library. All its interfaces should consider generic requirements not the one who depend on it.

For lightning on SQL, it's not a requirement for TiDB to open an external storage. So if we emplace this restriction developers will use the lightning configuration to set loader dir and checkpoint DSN, which will reuse more logic about binary lightning and library lightning. In other words, my WithXXX options only want to make least changes to API.

I would like that lightning binary and library use the same implementation to create the RestoreController later (This can be improved later). WithXXX options indeed doesn't introduce much changes, but it is more like a hack that is similar to failpoint.

use the same implementation to create the RestoreController.

For binary, parse config file, create external storage and pass to RestoreController.

For library, pass external storage to RestoreController directly.

Signed-off-by: lance6716 <[email protected]>

lance6716 · 2022-03-23T07:13:58Z

ping @WizardXiao

lance6716 · 2022-03-24T02:51:22Z

/cc @kennytm

lance6716 · 2022-03-25T03:41:57Z

/merge

ti-chi-bot · 2022-03-25T03:42:02Z

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

Commit hash: 1a11964

lance6716 · 2022-03-25T06:33:58Z

/run-unit-test
/run-mysql-test

lance6716 · 2022-03-25T06:34:42Z

/run-unit-tests

lance6716 · 2022-03-25T06:35:08Z

/run-mysql-tests

hawkingrei · 2022-03-25T06:49:38Z

/merge

lance6716 · 2022-03-25T06:54:33Z

/run-mysql-tests

ti-chi-bot · 2022-03-25T07:32:39Z

@lance6716: 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.

hawkingrei · 2022-03-25T07:36:05Z

br/cmd/tidb-lightning/main.go

@@ -22,12 +22,13 @@ import (
 	"runtime/debug"
 	"syscall"

+	"go.uber.org/zap"
+


remove this empty line.

Signed-off-by: lance6716 <[email protected]>

lance6716 · 2022-03-25T07:56:59Z

/merge

ti-chi-bot · 2022-03-25T07:57:02Z

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

Commit hash: 5ac26f0

hawkingrei · 2022-03-25T08:03:17Z

/merge

hawkingrei · 2022-03-25T08:12:51Z

/hold

hawkingrei · 2022-03-25T08:20:34Z

/unhold

lance6716 · 2022-03-25T08:22:23Z

/run-unit-tests

lance6716 · 2022-03-25T08:40:40Z

/run-unit-tests

lightning: support inject external storage when as library

3e34d96

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 22, 2022

add test

34b1c89

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 22, 2022

lance6716 changed the title ~~[WIP]lightning: support inject external storage when as library~~ lightning: support inject external storage when as library Mar 22, 2022

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 22, 2022

lance6716 requested review from WizardXiao and sleepymole March 22, 2022 06:01

remove debug message

a8ff409

Signed-off-by: lance6716 <[email protected]>

sleepymole reviewed Mar 22, 2022

View reviewed changes

address comment

17328ab

Signed-off-by: lance6716 <[email protected]>

WizardXiao reviewed Mar 22, 2022

View reviewed changes

lance6716 added 2 commits March 22, 2022 17:50

Merge branch 'master' of github.com:pingcap/tidb into lightning-extst…

a8676cc

…orage

address comment

a7011e6

Signed-off-by: lance6716 <[email protected]>

lance6716 force-pushed the lightning-extstorage branch from 8063bd8 to a7011e6 Compare March 22, 2022 10:00

sleepymole reviewed Mar 22, 2022

View reviewed changes

address comment

6e9073d

Signed-off-by: lance6716 <[email protected]>

sleepymole reviewed Mar 23, 2022

View reviewed changes

address comment

1a11964

Signed-off-by: lance6716 <[email protected]>

sleepymole approved these changes Mar 23, 2022

View reviewed changes

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

ti-chi-bot requested a review from kennytm March 24, 2022 02:51

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

ti-chi-bot added 2 commits March 25, 2022 11:42

Merge branch 'master' into lightning-extstorage

d5e2a6a

Merge branch 'master' into lightning-extstorage

0c4917d

ti-chi-bot added 2 commits March 25, 2022 15:02

Merge branch 'master' into lightning-extstorage

8d215c6

Merge branch 'master' into lightning-extstorage

b926161

hawkingrei reviewed Mar 25, 2022

View reviewed changes

address comment

5ac26f0

Signed-off-by: lance6716 <[email protected]>

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

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

hawkingrei approved these changes Mar 25, 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 Mar 25, 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 Mar 25, 2022

ti-chi-bot merged commit 96a507e into pingcap:master Mar 25, 2022

sleepymole mentioned this pull request Mar 28, 2022

lightning: fix test lightning_checkpoint_chunks #33503

Merged

1 task

D3Hunter mentioned this pull request Apr 6, 2022

lightning: enhance request log in server-mode and restore progress #33718

Merged

12 tasks

lightning: support inject external storage when as library #33303

lightning: support inject external storage when as library #33303

Conversation

lance6716 commented Mar 22, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 22, 2022 • edited Loading

sre-bot commented Mar 22, 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

Choose a reason for hiding this comment

sleepymole Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

lance6716 Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WizardXiao left a comment • edited Loading

Choose a reason for hiding this comment

lance6716 commented Mar 22, 2022

sleepymole 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

Choose a reason for hiding this comment

lance6716 commented Mar 23, 2022

lance6716 commented Mar 24, 2022

lance6716 commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

lance6716 commented Mar 25, 2022

lance6716 commented Mar 25, 2022

lance6716 commented Mar 25, 2022

hawkingrei commented Mar 25, 2022

lance6716 commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

hawkingrei commented Mar 25, 2022

hawkingrei commented Mar 25, 2022

hawkingrei commented Mar 25, 2022

lance6716 commented Mar 25, 2022

lance6716 commented Mar 25, 2022

lance6716 commented Mar 22, 2022 •

edited

Loading

ti-chi-bot commented Mar 22, 2022 •

edited

Loading

sre-bot commented Mar 22, 2022 •

edited

Loading

sleepymole Mar 22, 2022 •

edited

Loading

lance6716 Mar 22, 2022 •

edited

Loading

lance6716 Mar 22, 2022 •

edited

Loading

WizardXiao left a comment •

edited

Loading