github-actions[bot] commented on code in PR #65330:
URL: https://github.com/apache/doris/pull/65330#discussion_r3600002907
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -286,6 +286,11 @@ void Recycler::instance_scanner_callback() {
// enqueue instances
std::lock_guard lock(mtx_);
for (auto& instance : instances) {
+ if (instance.status() == InstanceInfoPB::DELETED &&
Review Comment:
[P1] Make the retained terminal record safe for rolling upgrades. This skip
and the new `COMPLETED` state are only understood by upgraded processes, but
the patch also stops removing the ordinary instance key. A pre-PR recycler
still enqueues every `DELETED` record: for legacy `obj_info` it can run the old
completion path and delete this status record, while for storage-vault-only
instances it repeatedly fails initialization after the new metadata phase has
removed the vault range. A pre-PR meta-service likewise treats key existence as
registration, and paths such as `begin_txn()` can create fresh metadata for the
deleted instance. Please add a rollout/version gate or store progress in a
representation old recycler/meta-service binaries cannot treat as a live
instance, with a mixed-version test.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -854,6 +864,31 @@ int InstanceRecycler::recycle_deleted_instance() {
<< "s, instance_id=" << instance_id_;
};
+ for (size_t retry_time = 0; retry_time < 10; retry_time++) {
+ switch (instance_info_.recycled_state()) {
+ case InstanceRecycleState::INSTANCE_RECYCLE_STATE_DATA_CLEANUP_PENDING:
+ ret = recycle_deleted_instance_data();
+ continue;
Review Comment:
[P2] Continue only after a successful phase transition. Both branches
currently `continue` regardless of `ret` or whether
`instance_info_.recycled_state()` changed. An active snapshot therefore reruns
tmp-rowset, versioned-rowset, and operation-log cleanup ten times in one job; a
referenced rowset or pending successor repeats its scans/transaction ten times;
errors retry immediately without backoff; and a `stopped()` return re-enters
DATA cleanup. Return errors immediately, return success on deliberate
no-progress, and continue only when the persisted phase actually advanced (with
call-count tests for these cases).
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -1020,33 +1070,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_COMPLETED,
Review Comment:
[P2] Define a retirement path for terminal records. Replacing instance-key
removal with a permanent `COMPLETED` PB means every deleted tenant remains in
the active instance range forever. `get_all_instances()` still range-reads,
allocates, and parses that whole range before recycler/checker/snapshot loops
filter terminal records, so recurring work grows with all historical deletions
rather than active tenants. Please add an acknowledgement/retention-based purge
or move durable tombstones out of the periodically scanned active range, and
test the terminal-record lifecycle.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -2562,6 +2562,9 @@ static std::pair<MetaServiceCode, std::string>
drop_single_instance(const std::s
instance->set_status(InstanceInfoPB::DELETED);
instance->set_mtime(duration_cast<seconds>(system_clock::now().time_since_epoch()).count());
+ instance->set_recycled_state(INSTANCE_RECYCLE_STATE_DATA_CLEANUP_PENDING);
Review Comment:
[P1] Preserve recycle progress when DROP is retried. Once cleanup reaches
`COMPLETED`, the instance PB is retained but its storage-vault keys have been
removed. A response-loss retry or repeated operator DROP reaches this line with
no already-`DELETED` guard and resets the record to `DATA_CLEANUP_PENDING`; the
next fresh worker then misses the terminal shortcut and returns `-2` while
initializing the now-empty vault range, so it can never advance again. Make
DROP idempotently retain the current state/timestamp (or reject an
already-deleted instance), and add a complete-then-repeat-DROP storage-vault
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]
