github-actions[bot] commented on code in PR #65249:
URL: https://github.com/apache/doris/pull/65249#discussion_r3528083598
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -2868,135 +2868,125 @@ int InstanceRecycler::recycle_tablets(int64_t
table_id, int64_t index_id,
.tag("num_recycled", num_recycled);
};
- // The first string_view represents the tablet key which has been recycled
- // The second bool represents whether the following fdb's tablet key
deletion could be done using range move or not
- using TabletKeyPair = std::pair<std::string_view, bool>;
- SyncExecutor<TabletKeyPair> sync_executor(
+ // The tablet key and id which have been recycled.
+ struct TabletInfo {
+ std::string_view tablet_meta_key;
+ int64_t tablet_id;
+ };
+ SyncExecutor<TabletInfo> sync_executor(
_thread_pool_group.recycle_tablet_pool,
fmt::format("recycle tablets, tablet id {}, index id {}, partition
id {}", table_id,
index_id, partition_id),
- [](const TabletKeyPair& k) { return k.first.empty(); });
+ [](const TabletInfo& k) { return k.tablet_meta_key.empty(); });
- // Elements in `tablet_keys` has the same lifetime as `it` in
`scan_and_recycle`
- std::vector<std::string> tablet_idx_keys;
- std::vector<std::string> restore_job_keys;
+ // Elements in `tablets_info` has the same lifetime as `it` in
`scan_and_recycle`
std::vector<std::string> init_rs_keys;
- std::vector<std::string> tablet_compact_stats_keys;
- std::vector<std::string> tablet_load_stats_keys;
- std::vector<std::string> versioned_meta_tablet_keys;
+ bool has_failure = false;
auto recycle_func = [&, this](std::string_view k, std::string_view v) ->
int {
- bool use_range_remove = true;
++num_scanned;
doris::TabletMetaCloudPB tablet_meta_pb;
if (!tablet_meta_pb.ParseFromArray(v.data(), v.size())) {
LOG_WARNING("malformed tablet meta").tag("key", hex(k));
- use_range_remove = false;
+ has_failure = true;
return -1;
}
int64_t tablet_id = tablet_meta_pb.tablet_id();
- if (!check_lazy_txn_finished(txn_kv_, instance_id_,
tablet_meta_pb.tablet_id())) {
+ if (config::enable_recycler_check_lazy_txn_finished &&
+ !check_lazy_txn_finished(txn_kv_, instance_id_,
tablet_meta_pb.tablet_id())) {
LOG(WARNING) << "lazy txn not finished tablet_id=" <<
tablet_meta_pb.tablet_id();
+ has_failure = true;
return -1;
}
- tablet_idx_keys.push_back(meta_tablet_idx_key({instance_id_,
tablet_id}));
- restore_job_keys.push_back(job_restore_tablet_key({instance_id_,
tablet_id}));
- if (is_multi_version) {
- // The tablet index/inverted index are recycled in
recycle_versioned_tablet.
- tablet_compact_stats_keys.push_back(
- versioned::tablet_compact_stats_key({instance_id_,
tablet_id}));
- tablet_load_stats_keys.push_back(
- versioned::tablet_load_stats_key({instance_id_,
tablet_id}));
- versioned_meta_tablet_keys.push_back(
- versioned::meta_tablet_key({instance_id_, tablet_id}));
- }
TEST_SYNC_POINT_RETURN_WITH_VALUE("recycle_tablet::bypass_check",
false);
- sync_executor.add([this, &num_recycled, tid = tablet_id, range_move =
use_range_remove,
- &metrics_context, k]() mutable -> TabletKeyPair {
- if (recycle_tablet(tid, metrics_context) != 0) {
- LOG_WARNING("failed to recycle tablet")
- .tag("instance_id", instance_id_)
- .tag("tablet_id", tid);
- range_move = false;
- return {std::string_view(), range_move};
- }
- ++num_recycled;
- LOG(INFO) << "recycle_tablets scan, key=" << (k.empty() ?
"(empty)" : hex(k));
- return {k, range_move};
- });
+ sync_executor.add(
+ [this, &num_recycled, tid = tablet_id, &metrics_context, k]()
-> TabletInfo {
+ if (recycle_tablet(tid, metrics_context) != 0) {
+ LOG_WARNING("failed to recycle tablet")
+ .tag("instance_id", instance_id_)
+ .tag("tablet_id", tid);
+ return {.tablet_meta_key = std::string_view(),
.tablet_id = tid};
+ }
+ ++num_recycled;
+ LOG(INFO) << "recycle_tablets scan, key=" << (k.empty() ?
"(empty)" : hex(k));
+ return {.tablet_meta_key = k, .tablet_id = tid};
+ });
return 0;
};
- // TODO(AlexYue): Add one ut to cover use_range_remove = false
auto loop_done = [&, this]() -> int {
+ int ret = 0;
bool finished = true;
- auto tablet_keys = sync_executor.when_all(&finished);
+ bool has_empty_key = false;
+ DORIS_CLOUD_DEFER {
+ init_rs_keys.clear();
+ has_failure = false;
+ };
+ auto tablets_info = sync_executor.when_all(&finished);
Review Comment:
A failed tablet still makes the production executor report `finished=false`
before this new partial-cleanup logic can run. The failure task returns an
empty `TabletInfo`, the cancel predicate treats that as cancellation, and
`SyncExecutor::Task` sets the stop token unless the test-only
`SyncExecutor::Task::bypass_cancel` hook is enabled. `when_all()` therefore
lands here with `finished=false` and returns before the later
`erase_if`/per-key deletion code can remove KVs for tablets that already
recycled successfully. The new regression test enables `bypass_cancel`, so it
proves the desired behavior only for the test-only path; in production the old
KV leak remains after any task failure. Please either make this executor
collect/process completed successful results even after one empty result, or
intentionally disable cancellation for this `recycle_tablets` executor in the
production code path as well.
--
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]
