gavinchou commented on code in PR #66870:
URL: https://github.com/apache/doris/pull/66870#discussion_r3809221527
##########
cloud/src/resource-manager/resource_manager.h:
##########
@@ -191,7 +191,7 @@ class ResourceManager {
*
* @param instance_id
*
- * @return true, instance_id in fdb kv
+ * @return true, instance_id exists in fdb kv and is not deleted
Review Comment:
the comm seems not correct
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -880,9 +888,6 @@ int InstanceRecycler::recycle_deleted_instance() {
case InstanceRecycleState::INSTANCE_RECYCLE_STATE_METADATA_CLEANUP_PENDING:
ret = recycle_deleted_instance_metadata();
break;
- case InstanceRecycleState::INSTANCE_RECYCLE_STATE_CLEANUP_COMPLETED:
- ret = remove_instance_key();
Review Comment:
ensure there is a log when removing instance in `remove_instance_key()`
##########
cloud/src/resource-manager/resource_manager.cpp:
##########
@@ -365,13 +382,21 @@ std::pair<bool, std::string>
ResourceManager::get_instance_id_by_cloud_unique_id
bool ResourceManager::is_instance_id_registered(const std::string&
instance_id) {
Review Comment:
`is_instance_id_registered()` returns true if the instance kv exists.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -856,6 +860,10 @@ int InstanceRecycler::recycle_deleted_instance() {
auto start_time = steady_clock::now();
const auto recycle_state = instance_info_.recycle_state();
+ if (recycle_state ==
InstanceRecycleState::INSTANCE_RECYCLE_STATE_CLEANUP_COMPLETED) {
Review Comment:
add a `config::retain_deleted_instance_tombstone`
##########
cloud/src/resource-manager/resource_manager.cpp:
##########
@@ -365,13 +382,21 @@ std::pair<bool, std::string>
ResourceManager::get_instance_id_by_cloud_unique_id
bool ResourceManager::is_instance_id_registered(const std::string&
instance_id) {
// check kv
- auto [c0, m0] = get_instance(nullptr, instance_id, nullptr);
+ InstanceInfoPB instance;
+ auto [c0, m0] = get_instance(nullptr, instance_id, &instance);
{ TEST_SYNC_POINT_CALLBACK("is_instance_id_registered", &c0); }
if (c0 != TxnErrorCode::TXN_OK) {
LOG(WARNING) << "failed to check instance instance_id=" << instance_id
<< ", code=" << format_as(c0) << ", info=" + m0;
+ return false;
}
- return c0 == TxnErrorCode::TXN_OK;
Review Comment:
this is a bug... if `c0` is not OK, we should not return false
return false **IFF** `c0 == NOT_FOUND`
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -996,17 +1001,31 @@ int
InstanceRecycler::recycle_deleted_instance_metadata() {
return -1;
}
+ InstanceInfoPB successor_instance;
std::string value;
err = txn->get(key, &value);
if (err == TxnErrorCode::TXN_OK) {
- LOG(INFO) << "instance successor instance is still exist, skip
deleting kv,"
- << " instance_id=" << instance_id_
- << " successor_instance_id=" <<
instance_info_.successor_instance_id();
- return 0;
+ InstanceInfoPB successor_instance;
+ if (!successor_instance.ParseFromString(value)) {
+ LOG(WARNING) << "failed to parse successor instance,
instance_id=" << instance_id_
+ << " successor_instance_id=" <<
instance_info_.successor_instance_id();
+ return -1;
+ }
+ if (successor_instance.status() != InstanceInfoPB::DELETED ||
+ successor_instance.recycle_state() !=
Review Comment:
ensure `successor_instance.recycle_state()` is not a fallback to
`INSTANCE_RECYCLE_STATE_CLEANUP_COMPLETED` when upgrade from older version,
which does not have `recycle_state`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
