github-actions[bot] commented on code in PR #65330:
URL: https://github.com/apache/doris/pull/65330#discussion_r3628118620
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -1020,33 +1071,79 @@ int InstanceRecycler::recycle_deleted_instance() {
std::string versioned_log_key_start =
versioned::log_key_prefix(instance_id_);
std::string versioned_log_key_end = versioned::log_key_prefix(instance_id_
+ '\x00');
txn->remove(versioned_log_key_start, versioned_log_key_end);
- err = txn->commit();
+
+ // Updating the recycle state also commits this transaction, making the
metadata deletions
+ // and state transition atomic.
+ if (update_instance_recycled_state(
+
InstanceRecycleState::INSTANCE_RECYCLE_STATE_CLEANUP_COMPLETED, txn.get()) !=
0) {
+ return -1;
+ }
+
+ return 0;
+}
+
+int InstanceRecycler::update_instance_recycled_state(InstanceRecycleState
recycled_state) {
+ std::unique_ptr<Transaction> txn;
+ TxnErrorCode err = txn_kv_->create_txn(&txn);
if (err != TxnErrorCode::TXN_OK) {
- LOG(WARNING) << "failed to delete all kv, instance_id=" <<
instance_id_ << ", err=" << err;
- ret = -1;
+ LOG(WARNING) << "failed to create txn";
+ return -1;
}
+ return update_instance_recycled_state(recycled_state, txn.get());
+}
- if (ret == 0) {
- // remove instance kv
- // ATTN: MUST ensure that cloud platform won't regenerate the same
instance id
- err = txn_kv_->create_txn(&txn);
- if (err != TxnErrorCode::TXN_OK) {
- LOG(WARNING) << "failed to create txn";
- ret = -1;
- return ret;
- }
- std::string key;
- instance_key({instance_id_}, &key);
- txn->atomic_add(system_meta_service_instance_update_key(), 1);
- txn->remove(key);
- err = txn->commit();
- if (err != TxnErrorCode::TXN_OK) {
- LOG(WARNING) << "failed to delete instance kv, instance_id=" <<
instance_id_
- << " err=" << err;
- ret = -1;
- }
+int InstanceRecycler::update_instance_recycled_state(InstanceRecycleState
recycled_state,
+ Transaction* txn) {
+ std::string key = instance_key({instance_id_});
+ std::string value;
+ TxnErrorCode err = txn->get(key, &value);
+ if (err != TxnErrorCode::TXN_OK) {
+ LOG_WARNING("failed to get instance when updating instance recycled
state")
+ .tag("instance_id", instance_id_)
+ .tag("recycled_state",
InstanceRecycleState_Name(recycled_state))
+ .tag("err", err);
+ return -1;
}
- return ret;
+
+ InstanceInfoPB instance;
+ if (!instance.ParseFromString(value)) {
+ LOG_WARNING("failed to parse InstanceInfoPB when updating instance
recycled state")
+ .tag("instance_id", instance_id_)
+ .tag("recycled_state",
InstanceRecycleState_Name(recycled_state));
+ return -1;
+ }
+ if (instance.status() != InstanceInfoPB::DELETED) {
+ LOG_WARNING("instance is not deleted when updating instance recycled
state")
+ .tag("instance_id", instance_id_)
+ .tag("status", instance.status())
+ .tag("recycled_state",
InstanceRecycleState_Name(recycled_state));
+ return -1;
+ }
+
+ instance.set_recycled_state(recycled_state);
Review Comment:
[P2] Keep recycle progress monotonic after lease takeover. A worker can lose
its lease while blocked in the final accessor's `delete_all()`;
`lease_recycle_jobs()` marks it stopped, but this path does not recheck
`stopped()` after `delete_all()` and opens this transaction afterward. If the
new owner has already committed `CLEANUP_COMPLETED`, this fresh read sees that
terminal PB and this assignment writes it back to `DATA_CLEANUP_COMPLETED`, so
FDB has no overlapping conflict to reject it and `check_instance_recycled` can
flip back to false. Require the expected old phase (`pending -> data complete`,
`data complete -> complete`), treating an equal/later persisted state
idempotently, and add a paused-old-worker/lease-takeover test.
##########
cloud/src/common/http_helper.cpp:
##########
@@ -424,6 +430,12 @@ const std::unordered_map<std::string_view,
HttpHandlerInfo>& get_http_handlers()
{.handler = [](void* s,
brpc::Controller* c) { return
process_update_config((MS*)s, c); },
.role = HttpRole::BOTH}},
+ {"check_instance_recycled",
+ {.handler =
+ [](void* s, brpc::Controller* c) {
+ return process_check_instance_recycled((RS*)s,
c);
Review Comment:
[P1] Do not dispatch this BOTH route through `RecyclerServiceImpl`.
`MetaServiceImpl::http` accepts `HttpRole::BOTH` and passes a
`MetaServiceImpl*` here, but this lambda casts it to the unrelated
`RecyclerServiceImpl*`; `check_instance_recycled()` immediately reads that
class's `txn_kv_`. The coincidentally similar first-member layout does not make
this a valid C++ object and can trap under CFI or read the wrong member after a
layout change. Use role-specific typed dispatchers/a common `TxnKv` helper (or
restrict the route) and exercise both service endpoints.
##########
cloud/src/resource-manager/resource_manager.cpp:
##########
@@ -1429,6 +1432,12 @@ void ResourceManager::refresh_instance(const
std::string& instance_id,
}
}
+ if (instance.status() == InstanceInfoPB::DELETED) {
+ instance_multi_version_status_.erase(instance_id);
+ instance_source_snapshot_info_.erase(instance_id);
Review Comment:
[P1] Preserve the clone source mapping until in-flight lazy commits finish.
DROP can mark rollback successor B deleted while an eventually committed txn is
already queued. That task retains `txn_info.versioned_read` and later builds
`CloneChainReader(B, shared ResourceManager)`; for an inherited tablet absent
from B it must follow B -> A@S. This erase can race the task, making the
fallback return `KEY_NOT_FOUND` even though A's metadata is deliberately
retained until B completes, leaving the committed txn non-visible. Drain/cancel
those tasks explicitly or snapshot/retain the source mapping until they finish,
with a paused lazy-commit/drop test.
--
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]
