github-actions[bot] commented on code in PR #66870:
URL: https://github.com/apache/doris/pull/66870#discussion_r3800976892
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -287,7 +287,12 @@ void Recycler::instance_scanner_callback() {
// enqueue instances
std::lock_guard lock(mtx_);
for (auto& instance : instances) {
- if (filter_out_instance(instance.instance_id())) continue;
+ if (filter_out_instance(instance.instance_id()) ||
Review Comment:
[P1] Keep completed tombstones out of active instance scans
This condition filters only after get_all_instances() has range-read and
deserialized every instance. Because completed keys are no longer removed, the
instance prefix grows with all deleted tenants forever; Recycler, both Checker
loops, SnapshotChainCompactor, and SnapshotDataMigrator repeat the full scan,
and their callers ignore get_all_instances() failures. Unlike
ResourceManager::init(), the helper does not recover from TXN_TOO_OLD, so once
churn makes a scan expire it can repeatedly process only a prefix and never
enqueue later live or deleting instances. Please keep the terminal marker out
of the active scan keyspace (or otherwise bound/exclude it at storage) and make
pagination renew or retry rather than accepting a partial result.
##########
cloud/src/resource-manager/resource_manager.cpp:
##########
@@ -109,6 +109,9 @@ int ResourceManager::init() {
std::unique_lock l(mtx_);
for (auto& [inst_id, inst] : instances) {
+ if (inst.status() == InstanceInfoPB::DELETED) {
Review Comment:
[P1] Keep terminal markers out of the legacy live-instance keyspace
This filter only protects upgraded readers. At the base revision, a cache
miss parses a degraded cloud unique ID and is_instance_id_registered() treats
any existing instance_key as live without parsing status; begin_txn() then
writes new tenant metadata under that ID. Full predecessor tombstones
additionally let old ResourceManager::init() populate a cache hit from retained
clusters. During a rolling upgrade, an old meta-server can therefore accept
writes after CLEANUP_COMPLETED, while upgraded recyclers permanently skip that
instance and never remove the recreated metadata. A clusterless PB under the
same key is still unsafe: store completion outside the legacy live-instance
keyspace (or gate retention until old consumers are gone) and add a
base-reader/current-writer test covering both cache-miss and restart/cache-hit
paths.
##########
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:
[P2] Reject mutations once the tombstone is terminal
Returning permanently for CLEANUP_COMPLETED leaves this instance key
reachable forever, but the shared alter_instance path does not enforce terminal
immutability: RENAME, ENABLE_SSE, DISABLE_SSE, SET_SNAPSHOT_PROPERTY, and
decouple_instance all load and rewrite a DELETED/CLEANUP_COMPLETED record
successfully. Previously the following recycle pass removed the key, so these
operations became not-found after cleanup. Please reject mutating operations at
the shared instance-mutation boundary once status is DELETED, while keeping
only the intended idempotent DROP, refresh, and completion-check behavior.
--
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]
