Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

restore: don't switch mode in tidb backend #368

Merged
merged 3 commits into from
Aug 13, 2020
Merged

restore: don't switch mode in tidb backend #368

merged 3 commits into from
Aug 13, 2020

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Aug 10, 2020

What problem does this PR solve?

Do not switch tikv to importer mode in tidb backend

What is changed and how it works?

Check List

Tests

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

Side effects

Related changes

@glorv glorv requested a review from kennytm August 10, 2020 10:43
overvenus
overvenus previously approved these changes Aug 11, 2020
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -460,15 +460,21 @@ func (rc *RestoreController) listenCheckpointUpdates() {
rc.checkpointsWg.Done()
}

func (rc *RestoreController) runPeriodicActions(ctx context.Context, stop <-chan struct{}) {
switchModeTicker := time.NewTicker(rc.cfg.Cron.SwitchMode.Duration)
func (rc *RestoreController) runPeriodicActions(ctx context.Context, stop <-chan struct{}, shouldSwitchMode bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i prefer removing the shouldSwitchMode parameter and check rc.cfg.TikvImporter.Backend inside this function

Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kennytm kennytm added the status/LGT1 One reviewer already commented LGTM (LGTM1) label Aug 11, 2020
@kennytm kennytm requested a review from overvenus August 11, 2020 14:22
Copy link
Contributor

@3pointer 3pointer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kennytm kennytm added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Aug 13, 2020
@glorv glorv merged commit f18863a into master Aug 13, 2020
@glorv glorv deleted the fix-switch-mode branch August 13, 2020 06:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants