Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: check scheduler's name from config file #1449

Merged
merged 1 commit into from Mar 5, 2019

Conversation

Projects
None yet
6 participants
@lerencao
Copy link
Contributor

lerencao commented Mar 4, 2019

What problem does this PR solve?

Check scheduler's name from configFile to prevent misspelled cases.

Related issue: #1443

What is changed and how it works?

Checking happens in scheduleConfigs validation stage.

Check List

Tests

  • Unit test
@sre-bot

This comment has been minimized.

Copy link

sre-bot commented Mar 4, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

1 similar comment
@sre-bot

This comment has been minimized.

Copy link

sre-bot commented Mar 4, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@lerencao lerencao force-pushed the lerencao:improvement/check-scheduler-typename branch 2 times, most recently from ba3d1c8 to ce06fa4 Mar 4, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@903e1e8). Click here to learn what that means.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1449   +/-   ##
=========================================
  Coverage          ?   67.97%           
=========================================
  Files             ?      158           
  Lines             ?    15208           
  Branches          ?        0           
=========================================
  Hits              ?    10338           
  Misses            ?     3954           
  Partials          ?      916
Impacted Files Coverage Δ
server/testutil.go 91.66% <ø> (ø)
server/coordinator.go 84.33% <0%> (ø)
server/config.go 82.14% <100%> (ø)
server/schedule/scheduler.go 66.66% <100%> (ø)

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 903e1e8...8a3b042. Read the comment docs.

@disksing

This comment has been minimized.

Copy link
Member

disksing commented Mar 4, 2019

/ok-to-test
/cc @Connor1996 @nolouch

@Connor1996
Copy link
Member

Connor1996 left a comment

use error log instead of panic when creating schedulers

config: check scheduler's name from config file
Also change log level from panic to error if fail to create schedulers.

@lerencao lerencao force-pushed the lerencao:improvement/check-scheduler-typename branch from ce06fa4 to 8a3b042 Mar 4, 2019

@lerencao

This comment has been minimized.

Copy link
Contributor Author

lerencao commented Mar 4, 2019

@Connor1996 Updated.

I intended to change it in another pr which will resolve the disabled invalid schedulers.
But it's ok to put it here.

@Connor1996
Copy link
Member

Connor1996 left a comment

LGTM

@nolouch

nolouch approved these changes Mar 5, 2019

@nolouch nolouch merged commit 4f86d93 into pingcap:master Mar 5, 2019

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@Connor1996

This comment has been minimized.

Copy link
Member

Connor1996 commented Mar 5, 2019

Fix #1443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.