Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

restore: auto remove/add pd scheduler in restore #123

Merged
merged 7 commits into from
Dec 24, 2019

Conversation

3pointer
Copy link
Collaborator

No description provided.

@3pointer 3pointer added the WIP label Dec 19, 2019
@3pointer
Copy link
Collaborator Author

/run-all-tests

4 similar comments
@3pointer
Copy link
Collaborator Author

/run-all-tests

@3pointer
Copy link
Collaborator Author

/run-all-tests

@3pointer
Copy link
Collaborator Author

/run-all-tests

@3pointer
Copy link
Collaborator Author

/run-all-tests

@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #123 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #123   +/-   ##
=======================================
  Coverage   73.81%   73.81%           
=======================================
  Files          33       33           
  Lines        3483     3483           
=======================================
  Hits         2571     2571           
  Misses        589      589           
  Partials      323      323

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6964d2...40603d2. Read the comment docs.

@3pointer 3pointer added type/feature-request New feature or request and removed WIP labels Dec 20, 2019
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.

Rest LGTM.

Are the schedulers still removed even in online import mode?

cmd/restore.go Outdated Show resolved Hide resolved
cmd/restore.go Outdated Show resolved Hide resolved
cmd/restore.go Outdated Show resolved Hide resolved
}
ctx, cancel := context.WithCancel(GetDefaultContext())
defer cancel()
func runRestore(flagSet *flag.FlagSet, cmdName, dbName, tableName string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍

pkg/conn/conn.go Outdated Show resolved Hide resolved
@3pointer
Copy link
Collaborator Author

Rest LGTM.

Are the schedulers still removed even in online import mode?

Sure, I think we should check is online in prepareWork

pkg/restore/client.go Outdated Show resolved Hide resolved
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

}

func addPDLeaderScheduler(ctx context.Context, mgr *conn.Mgr, removedSchedulers []string) error {
for _, scheduler := range removedSchedulers {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question. Since map is randomly ordered, the schedulers are added in random order as well. Is this OK for PD?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, yes, there is no dependency problem between each scheduler.

@kennytm kennytm added the status/LGT1 LGTM1 label Dec 23, 2019
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.

:shipit:

@overvenus overvenus merged commit 4bdb5b7 into pingcap:master Dec 24, 2019
overvenus pushed a commit to overvenus/br-1 that referenced this pull request Dec 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT1 LGTM1 type/feature-request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants