api/store:set state do not support set from or to Tombstone anymore#3407
Conversation
Signed-off-by: shirly <AndreMouche@126.com>
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
Codecov Report
@@ Coverage Diff @@
## master #3407 +/- ##
==========================================
+ Coverage 74.84% 75.02% +0.18%
==========================================
Files 243 243
Lines 23422 23431 +9
==========================================
+ Hits 17530 17580 +50
+ Misses 4311 4283 -28
+ Partials 1581 1568 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
| func TombstoneStore() StoreCreateOption { | ||
| return func(store *StoreInfo) { | ||
| meta := proto.Clone(store.meta).(*metapb.Store) | ||
| meta.State = metapb.StoreState_Tombstone |
There was a problem hiding this comment.
Should PhysicallyDestroyed be changed to false? And maybe we should use func setStoreState(state metapb.StoreState, physicallyDestroyed bool) StoreCreateOption to reduce redundant code.
There was a problem hiding this comment.
Agreed, the api design lgtm while we can use setStoreState to reduce the duplicated code.
There was a problem hiding this comment.
As PR #3388 discussed, SetStoreState are replaced by following StoreCreateOptions:
- UpStore()
- OfflineStore(physicallyDestroyed bool)
- TombstoneStore()
which have clearer definition and logic for each StoreCreateOption
| func TombstoneStore() StoreCreateOption { | ||
| return func(store *StoreInfo) { | ||
| meta := proto.Clone(store.meta).(*metapb.Store) | ||
| meta.State = metapb.StoreState_Tombstone |
There was a problem hiding this comment.
Agreed, the api design lgtm while we can use setStoreState to reduce the duplicated code.
Signed-off-by: shirly <AndreMouche@126.com>
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. DetailsReviewer can indicate their review by writing |
ti-chi-bot
added
status/LGT2
|
/merge |
|
@Yisaer: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests DetailsInstructions 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. |
|
This pull request has been accepted and is ready to merge. DetailsCommit hash: e3dbdde |
ti-chi-bot
added
the
status/can-merge
|
@AndreMouche: Your PR has out-of-dated, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests DetailsInstructions 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. |
| @@ -185,7 +185,7 @@ func (mc *Cluster) SetStoreDisconnect(storeID uint64) { | |||
| func (mc *Cluster) SetStoreDown(storeID uint64) { | |||
There was a problem hiding this comment.
I guess the function name is correct, but the implementation is incorrect before:(
6 participants
Signed-off-by: shirly AndreMouche@126.com
What problem does this PR solve?
To close issue #3076
This PR does not support setting a store's state to or from
Tombstoneby the APIset store stateWhat is changed and how it works?
Tombstoneanymore:state query string true "state" Enums(Up, Offline),
Tombstoneis not supported anymore. Also, this PR does not support to up a store fromtombstoneorphysically destroyedCheck List
Tests
Related changes
Release note