*: reset config if the input is invalid #8632
Conversation
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
ti-chi-bot
bot
added
release-note-none
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
| @@ -813,6 +813,60 @@ func (suite *schedulerTestSuite) checkSchedulerDiagnostic(cluster *pdTests.TestC | |||
| checkSchedulerDescribeCommand("balance-leader-scheduler", "normal", "") | |||
| } | |||
|
|
|||
| func (suite *schedulerTestSuite) TestEvictLeaderScheduler() { | |||
| // FIXME: API mode may have the problem | |||
There was a problem hiding this comment.
Will we fix it in another PR?
ti-chi-bot
bot
added
needs-1-more-lgtm
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8632 +/- ##
==========================================
+ Coverage 77.63% 77.65% +0.02%
==========================================
Files 474 474
Lines 62097 62105 +8
==========================================
+ Hits 48209 48228 +19
+ Misses 10340 10337 -3
+ Partials 3548 3540 -8
Flags with carried forward coverage won't be shown. Click here to find out more. |
| re.Contains(string(output), "Success!") | ||
| output, err = tests.ExecuteCommand(cmd, []string{"-u", pdAddr, "scheduler", "remove", "evict-leader-scheduler"}...) | ||
| re.NoError(err) | ||
| re.Contains(string(output), "Success!") |
There was a problem hiding this comment.
Maybe we need to check
testutil.Eventually(re, func() bool { // wait for removed scheduler to be synced to scheduling server.
output := mustExec(re, cmd, []string{"-u", pdAddr, "scheduler", "config", "evict-leader-scheduler"}, nil)
return strings.Contains(output, "[404] scheduler not found")
})
| @@ -408,6 +415,7 @@ func (handler *evictLeaderHandler) updateConfig(w http.ResponseWriter, r *http.R | |||
| batchFloat, ok := input["batch"].(float64) | |||
| if ok { | |||
| if batchFloat < 1 || batchFloat > 10 { | |||
| handler.config.removeStore(id) | |||
There was a problem hiding this comment.
For invalid config parameters, It's better to validate it, do nothing, and return 4xx + error message to let the users know it, it's not classic and unsafe to reset memory content when meeting invalid inputs.
There was a problem hiding this comment.
It's a rollback operation since the memory state has been changed in line 407.
| @@ -408,6 +415,7 @@ func (handler *evictLeaderHandler) updateConfig(w http.ResponseWriter, r *http.R | |||
| batchFloat, ok := input["batch"].(float64) | |||
| if ok { | |||
| if batchFloat < 1 || batchFloat > 10 { | |||
| handler.config.removeStore(id) | |||
There was a problem hiding this comment.
- Do we allow the input to not contain the store id? If this, we can check
if !inputHasStoreIDin L405, and assume that all subsequent cases areinputHasStoreIDby default. - How about puting L407(pauseLeaderTransferIfStoreNotExist) behind all checks over? In this case, we don't need to
removeStorefor each case.
There was a problem hiding this comment.
- Yes, we allow only changing batch size.
- range relies on
exist, only batch can be moved, but no much difference I think.
There was a problem hiding this comment.
How about adding a defer after L411? For now, it is so easy to forget to removeStore, like L445.
defer func () {
if err != nil {
....
}
}()
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, okJiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
ti-chi-bot
bot
added
needs-cherry-pick-release-7.1
ti-chi-bot
bot
added
needs-cherry-pick-release-8.1
|
In response to a cherrypick label: new pull request created to branch |
|
In response to a cherrypick label: new pull request created to branch |
close tikv#8619 Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
close tikv#8619 Signed-off-by: ti-chi-bot <[email protected]>
close tikv#8619 Signed-off-by: ti-chi-bot <[email protected]>
close tikv#8619 Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
close tikv#8619 Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
|
In response to a cherrypick label: new pull request created to branch |
close tikv#8619 Signed-off-by: ti-chi-bot <[email protected]>
close #8619 Signed-off-by: ti-chi-bot <[email protected]> Signed-off-by: Ryan Leung <[email protected]> Co-authored-by: Ryan Leung <[email protected]>
What problem does this PR solve?
Issue Number: Close #8619.
What is changed and how does it work?
Check List
Tests
Release note