restore: add `glue.Glue` interface and other function #456

lance6716 · 2020-11-06T08:32:53Z

What problem does this PR solve?

To embed lightning into TiDB, a glue interface is needed to use host TiDB's connection. This PR moves some external TiDB usage into a glue interface, to support hiding all call to external TiDB behind glue interface with following PRs.

What is changed and how it works?

add glue.Glue and glue.SQLExecutor. glue.SQLExecutor could do some SQL jobs.
add some call to Glue.Record to report information to TiDB
change RunOnce to enable using lightning as a library more friendly

Check List

Tests

Pass original test

Side effects

Possible performance regression
Increased code complexity

Related changes

lightning/common/util.go

lance6716 · 2020-11-08T12:44:21Z

/run-all-tests

Failed connect to 127.0.0.1:9000; Connection refused
Failed to start minio

lightning/glue/glue.go

lightning/restore/tidb.go

lance6716 · 2020-11-09T05:34:35Z

going to let TiDB implement a "glue" CheckpointsDB, here's the change of glue interface follwing this PR.
https://github.com/lance6716/tidb-lightning/pull/1/files

PTAL @overvenus

and PTAL for this PR @kennytm @glorv

*: more glue

lance6716 · 2020-11-13T06:36:04Z

/run-all-tests

cmd/tidb-lightning/main.go

lightning/glue/glue.go

glorv · 2020-11-13T07:03:31Z

lightning/lightning.go

 	})

+	if err := taskCfg.TiDB.Security.RegisterMySQL(); err != nil {


Are there any problems if we run multi lightning tasks with in the same tidb? Or tidb will refuse to run more than one lightning tasks?

currently TiDB will only run one task, and RunOnce is blocked thus it's unnatural to concurrently call RunOnce on one lightning (I hope).

For multiple lightning in one process, RegisterMySQL may fail because tlsConfigRegistry is a global variable, I'll be careful for that

glorv · 2020-11-13T07:14:42Z

lightning/lightning.go

@@ -284,6 +312,11 @@ func (l *Lightning) run(taskCfg *config.Config) (err error) {
 }

 func (l *Lightning) Stop() {
+	l.cancelLock.Lock()


This explicit cancel is not needed? If lightning run in server mod, the cancel task action will call this cancel? If lightning run in once mod, l.shutdown will stop lightning

A test expects this behaviour, that when lightning shutdown task should be canceled to avoid futhur actions (it's remove checkpoint here)

tidb-lightning/tests/checkpoint_chunks/run.sh

Lines 76 to 78 in ccf7f3c

# Set the failpoint to kill the lightning instance as soon as one chunk is imported, via signal mechanism

# If checkpoint does work, this should only kill $CHUNK_COUNT instances of lightnings.

export GO_FAILPOINTS="$TASKID_FAILPOINTS;github.com/pingcap/tidb-lightning/lightning/restore/KillIfImportedChunk=return($ROW_COUNT)"

glorv · 2020-11-13T07:18:46Z

lightning/log/log.go

@@ -106,6 +106,11 @@ func L() Logger {
 	return appLogger
 }

+// SetAppLogger replaces the default logger in this package to given one
+func SetAppLogger(l *zap.Logger) {
+	appLogger = Logger{l.WithOptions(zap.AddStacktrace(zap.DPanicLevel))}


They are not the same. #96~98 is the default operation to build an app log. But if we build a logger and set it as global, the caller should take responsibility to set all options and lightning shouldn't override its config anymore.

glorv · 2020-11-13T07:33:16Z

cmd/tidb-lightning/main.go

+			if err := cfg.LoadFromGlobal(globalCfg); err != nil {
+				return err
+			}
+			return app.RunOnce(context.Background(), cfg, nil, nil)


I have thought about it again. Since there is already a context in the Lightning struct RunOnce method can always depend on the app.ctx instead of another ctx as parameter?

In future usage, lightning may be New once (controlled by inner l.ctx) and that instance RunOnce for many times, like a HTTP server mode. So a task context will help

kennytm

rest LGTM

lightning/restore/restore.go

glorv

LGTM

add notes save work save work fix unit test remove tidbMgr in RestoreController remove some comments remove some comments change logger in SQLWithRetry revert replace log.Logger to *zap.Logger dep: update uuid dependency to latest google/uuid (pingcap#452) * dep: update satori/go.uuid to latest * fix tests * change to google/uuid * fix build * try fix test * get familiar with google/uuid * address comment tidb-lightning-ctl: change default of -d to 'noop://' (pingcap#453) also add noop:// to supported storage types (to represent an empty store) replace tab to space try another port to fix CI remove some comment *: more glue restore: fix the bug that gc life time ttl does not take effect (pingcap#448) * fix gc ttl loop * resolve comment and add tests fix CI report info to host TiDB config: filter out all system schemas by default (pingcap#459) backend: fix auto random default value for primary key (pingcap#457) * fix auto generate auto random primary key column * fix default for auto random primary key * fix test * use prev row id for auto random and add a test * replace chunck with session opt * fix * fix mydumper: fix parquet data parser (pingcap#435) * fix parquet * reorder imports * fix test * use empty collation * fix a error and add more test cases * add pointer type tests * resolve comments Co-authored-by: kennytm <kennytm@gmail.com> address comment backend/local: use range properties to optimize region range estimate (pingcap#422) * use range propreties to estimate region range * post-restore: add optional level for post-restore operations (pingcap#421) * add optional level for opst-restore operations * trim leading and suffix '" * use UnmarshalTOML to unmarshal post restore op level * resolve comments and fix unit test * backend/local: do not retry epochNotMatch error when ingest sst (pingcap#419) * do not retry epochNotMatch error when ingest sst * add retry ingest for 'Raft raft: proposal dropped' error in ingest * change some retryable error log level from Error to Warn * fix nextKey * add a comment for nextKey * fix comment and add a unit test * wrap time.Sleep in select Co-authored-by: kennytm <kennytm@gmail.com> * update * use range properties to optimze region range estimate * update pebble * change the default value for batch-size * add unit tests and reslove comments * add a comment to range properties test * add a comment * add a test for range property with pebble * rename const variable Co-authored-by: kennytm <kennytm@gmail.com> fix pd service id is empty (pingcap#460) fix s3 parquet reader (pingcap#461) Co-authored-by: Neil Shen <overvenus@gmail.com> fix service gc ttl again (pingcap#465) address comment mydumper: verify file routing config (pingcap#470) * fix file routing * remove useless line * remove redundant if check rename a method in interface save work try fix CI could work change ctx usage try fix CI try fix CI refine function interface refine some fucntion interface debug CI address comment config: allow four byte-size config to be specified using human-readable units ("100 GiB") (pingcap#471) * Makefile: add `make finish-prepare` action * config: accept human-readable size for most byte-related config e.g. allow `region-split-size = '96M'` in additional to `= 100663296` (known issue: these values' precisions will be truncated to 53 bits instead of supporting all 63 bits) * restore: reduce chance of spurious errors from TestGcTTLManagerSingle Co-authored-by: glorv <glorvs@163.com> remove debug log test: change double type syntax (pingcap#474) address comment checkpoint: add glue checkpoint resolve cycle import expose Retry refine change interface to cope with TiDB fix SQL string fix SQL adjust interface to embedded in TiDB could import now reduce TLS restore: add `glue.Glue` interface and other function (pingcap#456) * save my work * add notes * save work * save work * fix unit test * remove tidbMgr in RestoreController * remove some comments * remove some comments * change logger in SQLWithRetry * revert replace log.Logger to *zap.Logger * replace tab to space * try another port to fix CI * remove some comment * *: more glue * report info to host TiDB * fix CI * address comment * address comment * rename a method in interface * save work * try fix CI * could work * change ctx usage * try fix CI * try fix CI * refine function interface * refine some fucntion interface * debug CI * address comment * remove debug log * address comment modify code add comment refine some code

* save my work add notes save work save work fix unit test remove tidbMgr in RestoreController remove some comments remove some comments change logger in SQLWithRetry revert replace log.Logger to *zap.Logger dep: update uuid dependency to latest google/uuid (#452) * dep: update satori/go.uuid to latest * fix tests * change to google/uuid * fix build * try fix test * get familiar with google/uuid * address comment tidb-lightning-ctl: change default of -d to 'noop://' (#453) also add noop:// to supported storage types (to represent an empty store) replace tab to space try another port to fix CI remove some comment *: more glue restore: fix the bug that gc life time ttl does not take effect (#448) * fix gc ttl loop * resolve comment and add tests fix CI report info to host TiDB config: filter out all system schemas by default (#459) backend: fix auto random default value for primary key (#457) * fix auto generate auto random primary key column * fix default for auto random primary key * fix test * use prev row id for auto random and add a test * replace chunck with session opt * fix * fix mydumper: fix parquet data parser (#435) * fix parquet * reorder imports * fix test * use empty collation * fix a error and add more test cases * add pointer type tests * resolve comments Co-authored-by: kennytm <kennytm@gmail.com> address comment backend/local: use range properties to optimize region range estimate (#422) * use range propreties to estimate region range * post-restore: add optional level for post-restore operations (#421) * add optional level for opst-restore operations * trim leading and suffix '" * use UnmarshalTOML to unmarshal post restore op level * resolve comments and fix unit test * backend/local: do not retry epochNotMatch error when ingest sst (#419) * do not retry epochNotMatch error when ingest sst * add retry ingest for 'Raft raft: proposal dropped' error in ingest * change some retryable error log level from Error to Warn * fix nextKey * add a comment for nextKey * fix comment and add a unit test * wrap time.Sleep in select Co-authored-by: kennytm <kennytm@gmail.com> * update * use range properties to optimze region range estimate * update pebble * change the default value for batch-size * add unit tests and reslove comments * add a comment to range properties test * add a comment * add a test for range property with pebble * rename const variable Co-authored-by: kennytm <kennytm@gmail.com> fix pd service id is empty (#460) fix s3 parquet reader (#461) Co-authored-by: Neil Shen <overvenus@gmail.com> fix service gc ttl again (#465) address comment mydumper: verify file routing config (#470) * fix file routing * remove useless line * remove redundant if check rename a method in interface save work try fix CI could work change ctx usage try fix CI try fix CI refine function interface refine some fucntion interface debug CI address comment config: allow four byte-size config to be specified using human-readable units ("100 GiB") (#471) * Makefile: add `make finish-prepare` action * config: accept human-readable size for most byte-related config e.g. allow `region-split-size = '96M'` in additional to `= 100663296` (known issue: these values' precisions will be truncated to 53 bits instead of supporting all 63 bits) * restore: reduce chance of spurious errors from TestGcTTLManagerSingle Co-authored-by: glorv <glorvs@163.com> remove debug log test: change double type syntax (#474) address comment checkpoint: add glue checkpoint resolve cycle import expose Retry refine change interface to cope with TiDB fix SQL string fix SQL adjust interface to embedded in TiDB could import now reduce TLS restore: add `glue.Glue` interface and other function (#456) * save my work * add notes * save work * save work * fix unit test * remove tidbMgr in RestoreController * remove some comments * remove some comments * change logger in SQLWithRetry * revert replace log.Logger to *zap.Logger * replace tab to space * try another port to fix CI * remove some comment * *: more glue * report info to host TiDB * fix CI * address comment * address comment * rename a method in interface * save work * try fix CI * could work * change ctx usage * try fix CI * try fix CI * refine function interface * refine some fucntion interface * debug CI * address comment * remove debug log * address comment modify code add comment refine some code * address comment * add some comments * fix CI and change CREATE TABLE

lance6716 added 10 commits November 6, 2020 10:23

save my work

b4aeb1e

add notes

a5666a6

save work

5b79eda

save work

51e8c0e

fix unit test

02f3ac5

remove tidbMgr in RestoreController

84d27f3

remove some comments

114d7f1

remove some comments

d13bc83

change logger in SQLWithRetry

22d2f4e

Merge branch 'master' into glue

c55f5aa

kennytm reviewed Nov 7, 2020

View reviewed changes

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

lance6716 added 2 commits November 8, 2020 20:14

revert replace log.Logger to *zap.Logger

d0d01b9

Merge branch 'glue' of github.com:lance6716/tidb-lightning into glue

4fc56ec

lance6716 added the status/WIP Work in progress label Nov 8, 2020

This comment has been minimized.

Sign in to view

lance6716 removed the status/WIP Work in progress label Nov 8, 2020

This comment has been minimized.

Sign in to view

kennytm reviewed Nov 8, 2020

View reviewed changes

lightning/glue/glue.go Outdated Show resolved Hide resolved

lightning/restore/tidb.go Show resolved Hide resolved

lance6716 added 4 commits November 9, 2020 10:03

replace tab to space

e85c0b2

try another port to fix CI

5811245

remove some comment

d75e0b9

*: more glue

19d3aa5

lance6716 added 3 commits November 9, 2020 15:08

report info to host TiDB

c264392

Merge pull request #1 from lance6716/glue-more

f50ecb0

*: more glue

Merge branch 'master' into glue

9e595e8

lance6716 added the status/WIP Work in progress label Nov 9, 2020

fix CI

dbee87d

lance6716 added status/PTAL This PR is ready for review. Add this label back after committing new changes status/WIP Work in progress and removed status/WIP Work in progress status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 12, 2020

lance6716 added 4 commits November 12, 2020 20:47

try fix CI

481dbf4

try fix CI

04a0b7f

refine function interface

8978d8b

refine some fucntion interface

70188c4

lance6716 added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP Work in progress labels Nov 13, 2020

debug CI

15376ff

glorv reviewed Nov 13, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

lance6716 added 4 commits November 13, 2020 18:14

address comment

ff5c065

Merge branch 'master' into glue

1b446e5

remove debug log

14cc270

Merge branch 'master' into glue

1da5464

kennytm reviewed Nov 16, 2020

View reviewed changes

lightning/restore/restore.go Outdated Show resolved Hide resolved

address comment

e880ad1

kennytm added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 16, 2020

glorv approved these changes Nov 16, 2020

View reviewed changes

glorv added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Nov 16, 2020

glorv merged commit a78137f into pingcap:master Nov 16, 2020

lance6716 mentioned this pull request Apr 11, 2023

lightning: remove glue pingcap/tidb#42911

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restore: add `glue.Glue` interface and other function #456

restore: add `glue.Glue` interface and other function #456

lance6716 commented Nov 6, 2020 •

edited

This comment has been minimized.

lance6716 commented Nov 8, 2020 •

edited

This comment has been minimized.

This comment has been minimized.

lance6716 commented Nov 9, 2020

lance6716 commented Nov 13, 2020

glorv Nov 13, 2020

lance6716 Nov 13, 2020

glorv Nov 13, 2020

lance6716 Nov 13, 2020 •

edited

glorv Nov 13, 2020

glorv Nov 13, 2020

lance6716 Nov 13, 2020 •

edited

This comment has been minimized.

kennytm left a comment

glorv left a comment

		})

		if err := taskCfg.TiDB.Security.RegisterMySQL(); err != nil {

	# Set the failpoint to kill the lightning instance as soon as one chunk is imported, via signal mechanism
	# If checkpoint does work, this should only kill $CHUNK_COUNT instances of lightnings.
	export GO_FAILPOINTS="$TASKID_FAILPOINTS;github.com/pingcap/tidb-lightning/lightning/restore/KillIfImportedChunk=return($ROW_COUNT)"

restore: add glue.Glue interface and other function #456

restore: add glue.Glue interface and other function #456

Conversation

lance6716 commented Nov 6, 2020 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

This comment has been minimized.

lance6716 commented Nov 8, 2020 • edited

This comment has been minimized.

This comment has been minimized.

lance6716 commented Nov 9, 2020

lance6716 commented Nov 13, 2020

glorv Nov 13, 2020

Choose a reason for hiding this comment

lance6716 Nov 13, 2020

Choose a reason for hiding this comment

glorv Nov 13, 2020

Choose a reason for hiding this comment

lance6716 Nov 13, 2020 • edited

Choose a reason for hiding this comment

glorv Nov 13, 2020

Choose a reason for hiding this comment

glorv Nov 13, 2020

Choose a reason for hiding this comment

lance6716 Nov 13, 2020 • edited

Choose a reason for hiding this comment

This comment has been minimized.

kennytm left a comment

Choose a reason for hiding this comment

glorv left a comment

Choose a reason for hiding this comment

restore: add `glue.Glue` interface and other function #456

restore: add `glue.Glue` interface and other function #456

lance6716 commented Nov 6, 2020 •

edited

lance6716 commented Nov 8, 2020 •

edited

lance6716 Nov 13, 2020 •

edited

lance6716 Nov 13, 2020 •

edited