tools: fixed remove broken copyset after remove peer #373
Conversation
src/chunkserver/copyset_service.cpp
Outdated
| if (!copysetNodeManager_->IsExist(poolId, copysetId)) { | ||
| response->set_status(COPYSET_OP_STATUS_COPYSET_NOTEXIST); | ||
| LOG(WARNING) << "Remove copyset node " << groupId << " not exist"; | ||
| } else if (!copysetNodeManager_->PurgeCopysetNodeData(poolId, copysetId)) { |
There was a problem hiding this comment.
Should we check copyset status before purge its data？
There was a problem hiding this comment.
Should we check copyset status before purge its data？
I think there is a bug in my implemention. Actually, the copyset node will not exist in the copysetNodeManager_ if its data has broken, because the copysetNodeManager_ will ignore the data broken's copyset in the load phase when chunkserver start. So we will never delete the copyset node success.
There was a problem hiding this comment.
Should we check copyset status before purge its data？
In the latest implementation, i will only delete the copyset which data has broken.
Wine93
changed the title
src/chunkserver/copyset_service.h
Outdated
| @@ -30,6 +30,9 @@ namespace chunkserver { | |||
|
|
|||
| using ::google::protobuf::RpcController; | |||
| using ::google::protobuf::Closure; | |||
| using COPYSET_OP_STATUS::COPYSET_OP_STATUS_COPYSET_IS_HEALTHY; | |||
There was a problem hiding this comment.
For legacy C enum define, its internal enumerators are already exposed to its outer namespace, you can use COPYSET_OP_STATUS_COPYSET_IS_HEALTHY directly without the prefix COPYSET_OP_STATUS::, so no need to use those using.
There was a problem hiding this comment.
For legacy C enum define, its internal enumerators are already exposed to its outer namespace, you can use
COPYSET_OP_STATUS_COPYSET_IS_HEALTHYdirectly without the prefixCOPYSET_OP_STATUS::, so no need to use those using.
yeap, it's a bad use of plain enums, i will improve it.
| @@ -35,6 +35,8 @@ DEFINE_string(peer, | |||
| "", "Id of the operating peer"); | |||
| DEFINE_string(new_conf, | |||
| "", "new conf to reset peer"); | |||
| DEFINE_bool(remove_copyset, false, "Whether need to remove broken copyset " | |||
There was a problem hiding this comment.
why default is false? if the broken copyset data keep there, there will some problem
There was a problem hiding this comment.
why default is false? if the broken copyset data keep there, there will some problem
Actually, it's a danger action to delete copyset even if it is a broken copyset, we want to let user know what he is doning now, instead of delete it silently.
BTW: The copyset contain multi contents: chunk data, raft { log, meta, snapshot }.
| const CopysetRequest* request, | ||
| CopysetResponse* response, | ||
| Closure* done) { | ||
| LOG(INFO) << "Receive delete broken copyset request"; |
There was a problem hiding this comment.
Add groupId in this LOG ?
The below logic has guranteed the group id will be print.
Update Summer-of-Code mentor for 2021 CoreDNS project
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List