github-actions[bot] commented on code in PR #67020:
URL: https://github.com/apache/doris/pull/67020#discussion_r3849102522
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -288,8 +296,28 @@ void Recycler::instance_scanner_callback() {
if (!instances.empty()) {
// enqueue instances
std::lock_guard lock(mtx_);
+ std::vector<InstanceInfoPB> completed_instances;
for (auto& instance : instances) {
if (filter_out_instance(instance.instance_id())) continue;
+ if (should_retain_deleted_instance_tombstone(instance)) {
+ completed_instances.push_back(instance);
+ continue;
Review Comment:
[P2] Preserve cache invalidation for every skipped tombstone
This `continue` suppresses more than the no-op recycle job. Before the
distributed job lease is attempted, every queued record constructs an
`InstanceRecycler`, whose constructor explicitly refreshes the recycler-only
`TxnLazyCommitter` `ResourceManager` because that cache receives no
instance-change notifications. For a `DELETED` instance, that refresh erases
its cached nodes, multi-version status, and source-snapshot entry. A long-lived
recycler that cached the instance while it was normal but missed its pending
phases can now encounter it only after another replica completed cleanup; if it
is outside the newest five, it will never be constructed again, so stale nodes
and per-instance mappings accumulate with deletion churn. Please preserve
invalidation for every skipped terminal record through a batched or indexed
path outside `Recycler::mtx_`, while limiting only the queued recycle jobs.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -288,8 +296,28 @@ void Recycler::instance_scanner_callback() {
if (!instances.empty()) {
// enqueue instances
std::lock_guard lock(mtx_);
+ std::vector<InstanceInfoPB> completed_instances;
for (auto& instance : instances) {
if (filter_out_instance(instance.instance_id())) continue;
+ if (should_retain_deleted_instance_tombstone(instance)) {
+ completed_instances.push_back(instance);
+ continue;
+ }
+ auto [_, success] =
pending_instance_set_.insert(instance.instance_id());
+ // skip instance already in pending queue
+ if (success) {
+ pending_instance_queue_.push_back(std::move(instance));
+ }
+ }
+ std::ranges::sort(completed_instances, [&](const auto& l,
const auto& r) {
Review Comment:
[P2] Select the retained tombstones before taking `mtx_`
Retained completed instance keys are intentionally never removed while this
config is enabled, so this population grows with tenant churn. This block now
deep-copies every full `InstanceInfoPB` and performs an `O(T log T)` sort while
holding the same mutex needed by all recycle workers to dequeue, by
lease/task-check threads to snapshot `recycling_instance_map_`, and by
shutdown. As `T` grows, the hourly scan can therefore pause scheduler and lease
progress merely to choose five records. Please do the top-five selection on the
scanner-local vector before acquiring `mtx_` (for example with a fixed-size
heap or partial selection) and keep the lock scope to the pending set/queue
updates.
--
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]
