github-actions[bot] commented on code in PR #67946:
URL: https://github.com/apache/doris/pull/67946#discussion_r4011409078
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -6010,23 +5958,38 @@ int InstanceRecycler::recycle_rowsets() {
}
} else {
num_compacted += rowset.type() == RecycleRowsetPB::COMPACT;
- if (rowset_meta->num_segments() > 0) { // Skip empty rowset
- rowsets.emplace_back(std::string(k), std::move(*rowset_meta));
- } else {
+ rowset_keys.emplace_back(k);
+ rowsets.emplace(rowset_meta->rowset_id_v2(),
std::move(*rowset_meta));
+ if (rowset_meta->num_segments() <= 0) { // Skip empty rowset
++num_empty_rowset;
- rowset_keys_without_data.emplace_back(k);
}
}
return 0;
};
- auto submit_delete_rowset_data_job = [&](std::vector<std::string>
rowset_keys,
- std::map<std::string,
RowsetMetaCloudPB> rowsets) {
- worker_pool->submit([&, rowset_keys_to_delete = std::move(rowset_keys),
- rowsets_to_delete = std::move(rowsets)]() {
+ auto loop_done = [&]() -> int {
+ std::vector<std::string> rowset_keys_to_delete;
+ // rowset_id -> rowset_meta
+ // store rowset id and meta for statistics rs size when delete
+ std::map<std::string, doris::RowsetMetaCloudPB> rowsets_to_delete;
+ std::vector<std::string> mark_keys_to_process;
+ std::vector<std::string> abort_job_keys_to_process;
+ rowset_keys_to_delete.swap(rowset_keys);
+ rowsets_to_delete.swap(rowsets);
+ mark_keys_to_process.swap(rowset_keys_to_mark_recycled);
+ abort_job_keys_to_process.swap(rowset_keys_to_abort_job);
+ if (!mark_keys_to_process.empty()) {
+ submit_batch_mark_rowsets_as_recycled_job<RecycleRowsetPB>(
+ *worker_pool, std::move(mark_keys_to_process));
+ }
+ if (!abort_job_keys_to_process.empty()) {
+ submit_recycle_prepare_rowsets_job(*worker_pool,
std::move(abort_job_keys_to_process),
+ &num_recycled);
+ }
+ worker_pool->submit([&, rowset_keys_to_delete =
std::move(rowset_keys_to_delete),
Review Comment:
[P1] Preserve the rowset work bounds when removing shuffle
This makes an entire range page the unit of queued deletion work. The range
limit is 10,000 rows, and the default 32-thread pool can retain 32 running plus
64 queued page captures before backpressure, with the scanner holding a blocked
97th—about 970,000 parsed rowset metas for one instance. Active workers can
also materialize paths for up to 320,000 rowsets instead of the previous
32,000. Because recycle keys are tablet-ordered, a large low-ID tablet delays
scanning every later tablet until its backlog is queued and, beyond that bound,
drained. Reverting shuffle does not require dropping the explicit per-job and
per-tablet limits; please retain them and restore a multi-page, multi-tablet
regression test.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -5931,8 +5879,8 @@ int InstanceRecycler::recycle_rowsets() {
// old version `RecycleRowsetPB` may has empty resource_id,
just remove the kv.
LOG(INFO) << "delete the recycle rowset kv that has empty
resource_id, key="
<< hex(k) << " value=" << proto_to_json(rowset);
- rowset_keys_without_data.emplace_back(k);
- return 0;
+ rowset_keys.emplace_back(k);
+ return -1;
Review Comment:
[P2] Keep metadata-only legacy cleanup successful
This record is explicitly handled by queuing its KV for deletion, and
`loop_done` still removes that key before the worker pool drains. Returning
`-1` here nevertheless poisons `scan_and_recycle`'s aggregate result, so
`do_recycle` finishes the entire instance job as unsuccessful and records an
error even though the compatibility record was cleaned up. The deleted
`recycle_rowsets_delete_old_empty_resource_id_kvs_with_normal_rowsets` test
previously required this path to return success. Please keep the success
contract and retain a focused regression test.
```suggestion
return 0;
```
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -6010,23 +5958,38 @@ int InstanceRecycler::recycle_rowsets() {
}
} else {
num_compacted += rowset.type() == RecycleRowsetPB::COMPACT;
- if (rowset_meta->num_segments() > 0) { // Skip empty rowset
- rowsets.emplace_back(std::string(k), std::move(*rowset_meta));
- } else {
+ rowset_keys.emplace_back(k);
+ rowsets.emplace(rowset_meta->rowset_id_v2(),
std::move(*rowset_meta));
+ if (rowset_meta->num_segments() <= 0) { // Skip empty rowset
Review Comment:
[P3] Classify the rowset before moving its protobuf
`*rowset_meta` was moved into the map on the preceding line, so this
accessor reads a valid-but-unspecified moved-from protobuf (the normal
generated move leaves the source empty). Non-empty formal rowsets can therefore
increment `num_recycled.empty_rowset`, corrupting the completion telemetry even
though deletion uses the intact destination copy. Capture/check
`num_segments()` before the move and add an oracle that distinguishes empty
from non-empty rowsets.
--
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]
