lightning: check and restore pd scheduler even if our task failed #1336

glorv · 2021-07-12T15:08:13Z

What problem does this PR solve?

Fix the bug that in concurrency import mode, if one or more lightning failed before import all tables finished, pd schedulers won't be restored.

What is changed and how it works?

This PR adds a new status to mark that one task is exit before finishing. A lightning instance will set its status to one of (checksum_skipping, checksumming, unfinished), other lightinng can check these statuses to make sure whether it is the last running instance. The last instance should recover the pd schedulers configs.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Side effects

Increased code complexity

Related changes

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

Release note

Fix the bug that in concurrency mode, pd schedulers may not be restored if one or more lightning failed.

Little-Wallace · 2021-07-13T03:42:48Z

pkg/lightning/restore/meta_manager.go

+
+ err := exec.Transact(ctx, "check and init task status", func(ctx context.Context, tx *sql.Tx) error {
+ // avoid override existing metadata if the meta is already inserted.
+ stmt := fmt.Sprintf(`INSERT IGNORE INTO %s (task_id, status) values (?, ?)`, m.tableName)


Why insert be fore select......

Ensure there is exact one row represent current lightning. Need the reset the task status if current task's status is exit_unfinished

Little-Wallace · 2021-07-13T03:49:58Z

pkg/lightning/restore/restore.go

@@ -1435,6 +1439,7 @@ func (rc *Controller) restoreTables(ctx context.Context) error {
 // finishSchedulers()
 // cancelFunc(switchBack)
 // finishFuncCalled = true
+ taskFinished = true


What is it used for?

If current task is exit before lightning finished (maybe met error or by user terminating), we should not clean up the task/table meta tables if all other lightning are finished

Oh, I see. finishSchedulers will be called before this method ending...... Emmmmmm, it seems to be difficult to understand

Little-Wallace · 2021-07-13T05:04:08Z

pkg/lightning/restore/meta_manager.go

 )

+func (m taskMetaStatus) realStatus() taskMetaStatus {
+ return m & (taskMetaStatusExitUnfinished - 1)


Why define a method realstatus ?

pkg/lightning/restore/meta_manager.go

Little-Wallace

LGTM

ti-chi-bot · 2021-07-13T12:23:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Little-Wallace
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.

overvenus · 2021-07-14T05:23:21Z

pkg/lightning/restore/restore.go

 // finishSchedulers()
 // cancelFunc(switchBack)
 // finishFuncCalled = true


Need clean up?

We originally want to restore pd schedulers and switch back tikv to normal mode after data import finished. Then the cluster can do possible rebalance during checksum and analyze. But In our test, these rebalance will bring non-trivial impact to checksum and analyze. So we need to investigate further to determine whether we can still do this. So I think we can keep these before coming up with a clear conclusion.

kennytm

rest LGTM

kennytm · 2021-07-15T08:44:38Z

pkg/lightning/restore/meta_manager.go

+
+ err := exec.Transact(ctx, "check and init task status", func(ctx context.Context, tx *sql.Tx) error {
+ // avoid override existing metadata if the meta is already inserted.
+ stmt := fmt.Sprintf(`INSERT INTO %s (task_id, status) values (?, ?) ON DUPLICATE KEY UPDATE state = ?`, m.tableName)


the comment feels outdated.

kennytm · 2021-07-15T08:51:01Z

pkg/lightning/restore/restore.go

@@ -105,6 +105,7 @@ const (
 task_id BIGINT(20) UNSIGNED NOT NULL,
 pd_cfgs VARCHAR(2048) NOT NULL DEFAULT '',
 status VARCHAR(32) NOT NULL,
+ state TINYINT(1) NOT NULL DEFAULT 0 COMMENT '0: normal, 1: exited before finish',


should the taskMetaTableName be changed?

otherwise if we used Lightning before, and this CREATE TABLE IF NOT EXISTS means the task_meta with the old table structure will be used

Not sure. But since we don't GA this feature and if lightning exited before finished, the current logic may still not be recover except manually drop the meta schema. We also don't recommend change lightning binary during one import task.

sleepymole

LGTM after we have a consensus on #1336 (comment)

sleepymole · 2021-07-28T08:16:22Z

/run-integration_test

sleepymole · 2021-07-28T08:30:41Z

/run-integration_test

sleepymole · 2021-07-28T09:16:12Z

Unstable configlist test will be fixed by #1393.

sleepymole · 2021-07-28T09:16:29Z

/run-integration_test

…re-schedule

…to check-restore-schedule

glorv · 2021-08-06T07:51:04Z

/merge

ti-chi-bot · 2021-08-06T07:51:07Z

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

Commit hash: 799f4e0

glorv · 2021-08-20T08:49:39Z

/cherry-pick release-5.1

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2021-08-20T08:49:57Z

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

ti-chi-bot · 2021-08-20T08:50:31Z

@glorv: new pull request could not be created: failed to create pull request against pingcap/br#release-5.1 from head ti-chi-bot:cherry-pick-1336-to-release-5.1: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for ti-chi-bot:cherry-pick-1336-to-release-5.1."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

In response to this:

/cherry-pick release-5.1

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.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

) (#1422)

check and restore pd scheduler even if our task failed

4f5e712

glorv requested review from kennytm, Little-Wallace and sleepymole July 12, 2021 15:08

ti-chi-bot added the size/L label Jul 12, 2021

glorv added component/import type/bug-fix type/cherry-pick-for-release-5.1 labels Jul 12, 2021

fix task meta status

09ba51d

Little-Wallace reviewed Jul 13, 2021

View reviewed changes

glorv added 2 commits July 13, 2021 14:58

move task exit state flag into a separate field

9c240b9

fix

a1d7b1c

kennytm reviewed Jul 13, 2021

View reviewed changes

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

Little-Wallace approved these changes Jul 13, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 LGTM1 label Jul 13, 2021

add named return values

49aaf55

overvenus reviewed Jul 14, 2021

View reviewed changes

kennytm reviewed Jul 15, 2021

View reviewed changes

sleepymole reviewed Jul 19, 2021

View reviewed changes

Merge branch 'master' into check-restore-schedule

504b9af

sleepymole approved these changes Jul 28, 2021

View reviewed changes

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

glorv added 4 commits August 6, 2021 15:16

Merge branch 'master' of ssh://github.com/pingcap/br into check-resto…

8e6e89c

…re-schedule

Merge branch 'master' into check-restore-schedule

c85a70b

fix conflict

a2f959c

Merge branch 'check-restore-schedule' of ssh://github.com/glorv/br in…

799f4e0

…to check-restore-schedule

ti-chi-bot added the status/can-merge label Aug 6, 2021

ti-chi-bot merged commit 1b0e54c into pingcap:master Aug 6, 2021

3pointer mentioned this pull request Aug 6, 2021

BR: update br keep upstream pingcap/tidb#26972

Merged

2 tasks

ti-srebot mentioned this pull request Aug 9, 2021

BR: update br keep upstream (#26972) pingcap/tidb#27003

Merged

2 tasks

glorv added needs-cherry-pick-release-5.1 and removed type/cherry-pick-for-release-5.1 labels Aug 20, 2021

ti-chi-bot pushed a commit to ti-chi-bot/br that referenced this pull request Aug 20, 2021

This is an automated cherry-pick of pingcap#1336

66cedb1

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Aug 20, 2021

lightning: check and restore pd scheduler even if our task failed (#1336) #1422

Merged

glorv deleted the check-restore-schedule branch August 20, 2021 08:50

ti-chi-bot pushed a commit to ti-chi-bot/br that referenced this pull request Aug 20, 2021

This is an automated cherry-pick of pingcap#1336

24d97f3

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot added a commit that referenced this pull request Sep 13, 2021

lightning: check and restore pd scheduler even if our task failed (#1336

1eb2415

) (#1422)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: check and restore pd scheduler even if our task failed #1336

lightning: check and restore pd scheduler even if our task failed #1336

glorv commented Jul 12, 2021

Little-Wallace Jul 13, 2021

glorv Jul 13, 2021

Little-Wallace Jul 13, 2021

glorv Jul 13, 2021 •

edited

Loading

Little-Wallace Jul 13, 2021

Little-Wallace Jul 13, 2021

Little-Wallace left a comment

ti-chi-bot commented Jul 13, 2021 •

edited

Loading

overvenus Jul 14, 2021

glorv Jul 14, 2021

kennytm left a comment

kennytm Jul 15, 2021

kennytm Jul 15, 2021

glorv Jul 19, 2021

sleepymole left a comment

sleepymole commented Jul 28, 2021

sleepymole commented Jul 28, 2021

sleepymole commented Jul 28, 2021

sleepymole commented Jul 28, 2021

glorv commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

glorv commented Aug 20, 2021

ti-chi-bot commented Aug 20, 2021

ti-chi-bot commented Aug 20, 2021

lightning: check and restore pd scheduler even if our task failed #1336

lightning: check and restore pd scheduler even if our task failed #1336

Conversation

glorv commented Jul 12, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glorv Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Little-Wallace left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm 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

sleepymole left a comment

Choose a reason for hiding this comment

sleepymole commented Jul 28, 2021

sleepymole commented Jul 28, 2021

sleepymole commented Jul 28, 2021

sleepymole commented Jul 28, 2021

glorv commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

glorv commented Aug 20, 2021

ti-chi-bot commented Aug 20, 2021

ti-chi-bot commented Aug 20, 2021

glorv Jul 13, 2021 •

edited

Loading

ti-chi-bot commented Jul 13, 2021 •

edited

Loading