You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "wyxxxcat (via GitHub)" <gi...@apache.org> on 2026/05/19 02:09:17 UTC
[PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
wyxxxcat opened a new pull request, #63377:
URL: https://github.com/apache/doris/pull/63377
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg: https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "liaoxin01 (via GitHub)" <gi...@apache.org>.
liaoxin01 commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4550927706
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4483882265
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4657408383
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4483839282
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on code in PR #63377:
URL: https://github.com/apache/doris/pull/63377#discussion_r3331563906
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -2906,108 +2905,110 @@ int InstanceRecycler::recycle_tablets(int64_t table_id, int64_t index_id,
}
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();
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 {
+ &metrics_context, k]() mutable -> TabletInfo {
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};
+ return {.tablet_meta_key = std::string_view(),
+ .tablet_id = tid,
+ .range_move = range_move};
}
++num_recycled;
LOG(INFO) << "recycle_tablets scan, key=" << (k.empty() ? "(empty)" : hex(k));
- return {k, range_move};
+ return {.tablet_meta_key = k, .tablet_id = tid, .range_move = range_move};
});
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;
+ auto tablets_info = sync_executor.when_all(&finished);
if (!finished) {
LOG_WARNING("failed to recycle tablet").tag("instance_id", instance_id_);
return -1;
}
- if (tablet_keys.empty() && tablet_idx_keys.empty()) return 0;
- if (!tablet_keys.empty() &&
- std::ranges::all_of(tablet_keys, [](const auto& k) { return k.first.empty(); })) {
- return -1;
+
+ size_t size_before_erase = tablets_info.size();
+ std::erase_if(tablets_info, [](const TabletInfo& t) { return t.tablet_meta_key.empty(); });
+ if (tablets_info.empty()) {
+ return size_before_erase == 0 ? 0 : -1;
+ } else if (size_before_erase != tablets_info.size()) {
+ has_empty_key = true;
}
+
+ ret = has_empty_key ? -1 : 0;
// sort the vector using key's order
- std::sort(tablet_keys.begin(), tablet_keys.end(),
- [](const auto& prev, const auto& last) { return prev.first < last.first; });
+ std::ranges::sort(tablets_info, [](const auto& prev, const auto& last) {
+ return prev.tablet_meta_key < last.tablet_meta_key;
+ });
bool use_range_remove = true;
- for (auto& [_, remove] : tablet_keys) {
- if (!remove) {
- use_range_remove = remove;
+ for (auto& tablet_info : tablets_info) {
+ if (!has_empty_key && !tablet_info.range_move) {
+ use_range_remove = tablet_info.range_move;
break;
}
}
DORIS_CLOUD_DEFER {
- tablet_idx_keys.clear();
- restore_job_keys.clear();
init_rs_keys.clear();
- tablet_compact_stats_keys.clear();
- tablet_load_stats_keys.clear();
- versioned_meta_tablet_keys.clear();
};
std::unique_ptr<Transaction> txn;
if (txn_kv_->create_txn(&txn) != TxnErrorCode::TXN_OK) {
LOG(WARNING) << "failed to delete tablet meta kv, instance_id=" << instance_id_;
return -1;
}
std::string tablet_key_end;
- if (!tablet_keys.empty()) {
+ if (!tablets_info.empty()) {
if (use_range_remove) {
- tablet_key_end = std::string(tablet_keys.back().first) + '\x00';
- txn->remove(tablet_keys.front().first, tablet_key_end);
+ tablet_key_end = std::string(tablets_info.back().tablet_meta_key) + '\x00';
+ txn->remove(tablets_info.front().tablet_meta_key, tablet_key_end);
} else {
- for (auto& [k, _] : tablet_keys) {
- txn->remove(k);
+ for (auto& tablet_info : tablets_info) {
+ txn->remove(tablet_info.tablet_meta_key);
}
}
}
if (is_multi_version) {
Review Comment:
@gavinchou here
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4484221956
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4539801314
# Cloud UT Coverage Report
Increment line coverage `90.57% (48/53)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/18ad9817c19505631b7a7ad26863ce1c2506cb66_18ad9817c19505631b7a7ad26863ce1c2506cb66_cloud/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/18ad9817c19505631b7a7ad26863ce1c2506cb66_18ad9817c19505631b7a7ad26863ce1c2506cb66_cloud/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 78.06% (1854/2375) |
| Line Coverage | 64.52% (33338/51669) |
| Region Coverage | 65.21% (16531/25351) |
| Branch Coverage | 55.69% (8835/15864) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4483881093
/reviewrun buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4591019841
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4541360131
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4591020436
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4539708223
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4483874073
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4542387846
OpenCode automated review failed and did not complete.
Error: Review step was failure (possibly timeout or cancelled)
Workflow run: https://github.com/apache/doris/actions/runs/26437267588
Please inspect the workflow logs and rerun the review after the underlying issue is resolved.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on code in PR #63377:
URL: https://github.com/apache/doris/pull/63377#discussion_r3315163563
##########
cloud/src/common/config.h:
##########
@@ -331,6 +331,7 @@ CONF_Int64(txn_lazy_commit_shuffle_seed, "0"); // 0 means generate a random seed
// When enabled, defer deleting pending delete bitmaps until lazy commit completes.
// This reduces contention during transaction commit by extending delete bitmap locks.
CONF_mBool(txn_lazy_commit_defer_deleting_pending_delete_bitmaps, "false");
+CONF_mBool(enable_recycler_check_lazy_txn_finished, "true");
// max TabletIndexPB num for batch get
CONF_Int32(max_tablet_index_num_per_batch, "1000");
Review Comment:
done
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "gavinchou (via GitHub)" <gi...@apache.org>.
gavinchou commented on code in PR #63377:
URL: https://github.com/apache/doris/pull/63377#discussion_r3331554239
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -2906,108 +2905,110 @@ int InstanceRecycler::recycle_tablets(int64_t table_id, int64_t index_id,
}
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();
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) {
Review Comment:
what's the corresponding logic to this in the modified code?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4539602908
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on code in PR #63377:
URL: https://github.com/apache/doris/pull/63377#discussion_r3331563258
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -2906,108 +2905,110 @@ int InstanceRecycler::recycle_tablets(int64_t table_id, int64_t index_id,
}
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();
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) {
Review Comment:
Similar to tablet_idx_keys, it only deletes the corresponding key-value pairs after the object is successfully deleted, rather than recording all key-value pairs.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #63377:
URL: https://github.com/apache/doris/pull/63377#discussion_r3332896676
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -2914,108 +2913,110 @@ int InstanceRecycler::recycle_tablets(int64_t table_id, int64_t index_id,
}
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();
Review Comment:
This skipped-tablet path is not represented in `tablets_info`, so the range delete below can still remove metadata for a tablet whose cleanup did not run. For example, if tablets A and C recycle successfully but tablet B returns here because `check_lazy_txn_finished()` is false, `loop_done()` only sees A/C, `has_empty_key` remains false, and the range remove from A through C can delete B's `meta_tablet_key` while B still has pending lazy commits and unrecycled data. This is distinct from the existing empty-result partial-failure thread because this path never adds a `TabletInfo` placeholder. Please add a failure placeholder or otherwise disable range deletion whenever a scanned tablet is skipped before `sync_executor.add()`.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4591271748
# Cloud UT Coverage Report
Increment line coverage `90.57% (48/53)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/8728fb8f2775e7343fd431e64bbada26f5ff56f8_8728fb8f2775e7343fd431e64bbada26f5ff56f8_cloud/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/8728fb8f2775e7343fd431e64bbada26f5ff56f8_8728fb8f2775e7343fd431e64bbada26f5ff56f8_cloud/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 78.34% (1906/2433) |
| Line Coverage | 64.83% (34017/52472) |
| Region Coverage | 65.36% (17543/26841) |
| Branch Coverage | 54.01% (9311/17240) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4539603431
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #63377:
URL: https://github.com/apache/doris/pull/63377#discussion_r3301029915
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -2906,108 +2905,110 @@ int InstanceRecycler::recycle_tablets(int64_t table_id, int64_t index_id,
}
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();
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 {
+ &metrics_context, k]() mutable -> TabletInfo {
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};
+ return {.tablet_meta_key = std::string_view(),
+ .tablet_id = tid,
+ .range_move = range_move};
}
++num_recycled;
LOG(INFO) << "recycle_tablets scan, key=" << (k.empty() ? "(empty)" : hex(k));
- return {k, range_move};
+ return {.tablet_meta_key = k, .tablet_id = tid, .range_move = range_move};
});
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;
+ auto tablets_info = sync_executor.when_all(&finished);
if (!finished) {
LOG_WARNING("failed to recycle tablet").tag("instance_id", instance_id_);
return -1;
}
- if (tablet_keys.empty() && tablet_idx_keys.empty()) return 0;
- if (!tablet_keys.empty() &&
- std::ranges::all_of(tablet_keys, [](const auto& k) { return k.first.empty(); })) {
- return -1;
+
+ size_t size_before_erase = tablets_info.size();
+ std::erase_if(tablets_info, [](const TabletInfo& t) { return t.tablet_meta_key.empty(); });
+ if (tablets_info.empty()) {
+ return size_before_erase == 0 ? 0 : -1;
+ } else if (size_before_erase != tablets_info.size()) {
+ has_empty_key = true;
}
+
+ ret = has_empty_key ? -1 : 0;
// sort the vector using key's order
- std::sort(tablet_keys.begin(), tablet_keys.end(),
- [](const auto& prev, const auto& last) { return prev.first < last.first; });
+ std::ranges::sort(tablets_info, [](const auto& prev, const auto& last) {
+ return prev.tablet_meta_key < last.tablet_meta_key;
+ });
bool use_range_remove = true;
- for (auto& [_, remove] : tablet_keys) {
- if (!remove) {
- use_range_remove = remove;
+ for (auto& tablet_info : tablets_info) {
+ if (!has_empty_key && !tablet_info.range_move) {
Review Comment:
This keeps `use_range_remove` true whenever any task failed (`has_empty_key == true`). In a mixed batch, the failed tablet key is erased from `tablets_info`; if the remaining successful keys sort around it, the later range remove deletes the failed tablet's `meta_tablet_key` even though `recycle_tablet()` returned failure and its data/versioned cleanup did not complete. For example successes A/C and failed B will call `remove(A, C + '\0')`, deleting B's tablet meta and making the failed cleanup non-retryable. Please force point deletes whenever `has_empty_key` is true, or return before deleting any tablet metadata in that batch.
##########
cloud/test/recycler_test.cpp:
##########
@@ -8694,6 +8680,160 @@ TEST(RecyclerTest, recycle_tablet_with_delete_file_failure) {
}
}
+TEST(RecyclerTest, recycle_tablet_with_delete_file_partial_failure) {
+ // If part of object deletion fails, recycle_tablets should delete KV only for tablets whose
+ // objects were deleted successfully, but still return failure for the whole round.
+
+ auto* sp = SyncPoint::get_instance();
+ DORIS_CLOUD_DEFER {
+ sp->clear_all_call_backs();
+ sp->clear_trace();
+ sp->disable_processing();
+ };
+
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+
+ InstanceInfoPB instance;
+ instance.set_instance_id(::instance_id);
+ auto obj_info = instance.add_obj_info();
+ obj_info->set_id("recycle_tablet_with_delete_file_failure");
+ obj_info->set_ak(config::test_s3_ak);
+ obj_info->set_sk(config::test_s3_sk);
+ obj_info->set_endpoint(config::test_s3_endpoint);
+ obj_info->set_region(config::test_s3_region);
+ obj_info->set_bucket(config::test_s3_bucket);
+ obj_info->set_prefix("recycle_tablet_with_delete_file_failure");
+
+ InstanceRecycler recycler(txn_kv, instance, thread_group,
+ std::make_shared<TxnLazyCommitter>(txn_kv));
+ ASSERT_EQ(recycler.init(), 0);
+
+ doris::TabletSchemaCloudPB schema;
+ schema.set_schema_version(1);
+
+ constexpr int64_t table_id = 21000;
+ constexpr int64_t index_id = 21001;
+ constexpr int64_t partition_id = 21002;
+ constexpr int64_t tablet_id = 21003;
+
+ auto accessor = recycler.accessor_map_.begin()->second;
+
+ // Create 5 tablet with its metadata and index keys
+ for (int i = 0; i < 5; ++i) {
+ ASSERT_EQ(create_tablet(txn_kv.get(), table_id, index_id, partition_id, tablet_id + i), 0);
+ }
+
+ // Create some committed rowsets for the tablet (in meta_rowset_key)
+ // These will be picked up by recycle_tablet when it scans for rowsets
+ for (int i = 0; i < 5; ++i) {
+ for (int j = 0; j < 5; j++) {
+ create_committed_rowset(txn_kv.get(), accessor.get(),
+ "recycle_tablet_with_delete_file_failure", tablet_id + i, j,
+ index_id, 2, 1, true);
+ }
+ }
+
+ // Create partition version kv (required for lazy txn check)
+ ASSERT_EQ(create_partition_version_kv(txn_kv.get(), table_id, partition_id), 0);
+
+ // Inject failure: make delete_directory return -1
+ // This simulates the scenario where accessor fails to delete directory from object storage
+ std::atomic<int> delete_directory_call_count {0};
+ size_t partial_cnt = 0;
+ sp->set_call_back("SyncExecutor::Task::bypass_cancel",
+ [](auto&& args) { *try_any_cast<bool*>(args[0]) = true; });
+ sp->set_call_back("MockAccessor::delete_prefix",
+ [&delete_directory_call_count, &partial_cnt](auto&& args) {
+ if (partial_cnt++ < 2) {
+ auto* ret = try_any_cast_ret<int>(args);
Review Comment:
This callback is invoked by multiple `recycle_tablet_pool` tasks concurrently, but `partial_cnt` is a plain `size_t`. The `partial_cnt++` is a data race and also makes the test nondeterministic about which tablet IDs fail, so the partial-failure coverage can be flaky. Please use an atomic counter, e.g. `std::atomic<size_t> partial_cnt {0};` with `fetch_add`, and make the intended failed/successful key layout deterministic enough to cover an interleaved failed key.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "liaoxin01 (via GitHub)" <gi...@apache.org>.
liaoxin01 commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4650544382
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "liaoxin01 (via GitHub)" <gi...@apache.org>.
liaoxin01 merged PR #63377:
URL: https://github.com/apache/doris/pull/63377
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4486135991
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "liaoxin01 (via GitHub)" <gi...@apache.org>.
liaoxin01 commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4657401823
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4483823060
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4483881628
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4486523012
# Cloud UT Coverage Report
Increment line coverage `93.88% (46/49)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/3e91342fac563a041eb23b4400ac984e5a1843e0_3e91342fac563a041eb23b4400ac984e5a1843e0_cloud/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/3e91342fac563a041eb23b4400ac984e5a1843e0_3e91342fac563a041eb23b4400ac984e5a1843e0_cloud/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 78.06% (1854/2375) |
| Line Coverage | 64.55% (33347/51662) |
| Region Coverage | 65.23% (16533/25345) |
| Branch Coverage | 55.75% (8839/15854) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #63377:
URL: https://github.com/apache/doris/pull/63377#issuecomment-4539747897
# Cloud UT Coverage Report
Increment line coverage `93.88% (46/49)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/200b645c1b169c1423982d4d2799cd338b457efd_200b645c1b169c1423982d4d2799cd338b457efd_cloud/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/200b645c1b169c1423982d4d2799cd338b457efd_200b645c1b169c1423982d4d2799cd338b457efd_cloud/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 78.06% (1854/2375) |
| Line Coverage | 64.53% (33340/51665) |
| Region Coverage | 65.22% (16532/25347) |
| Branch Coverage | 55.75% (8839/15856) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Prevent tablet KV leak after partial recycle failure [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #63377:
URL: https://github.com/apache/doris/pull/63377#discussion_r3308289944
##########
cloud/src/common/config.h:
##########
@@ -331,6 +331,7 @@ CONF_Int64(txn_lazy_commit_shuffle_seed, "0"); // 0 means generate a random seed
// When enabled, defer deleting pending delete bitmaps until lazy commit completes.
// This reduces contention during transaction commit by extending delete bitmap locks.
CONF_mBool(txn_lazy_commit_defer_deleting_pending_delete_bitmaps, "false");
+CONF_mBool(enable_recycler_check_lazy_txn_finished, "true");
// max TabletIndexPB num for batch get
CONF_Int32(max_tablet_index_num_per_batch, "1000");
Review Comment:
This switch makes a recycler correctness invariant optional. If an operator disables it while a dropped tablet still has pending lazy-commit transactions, `recycle_tablets()` and `scan_tablets_and_statistics()` will proceed to delete object data and tablet metadata before those lazy commits finish. A later lazy commit can then publish/advance metadata for rowsets whose files or tablet KV were already removed, making the cleanup non-retryable and risking data loss/inconsistent commit state. The recycler should always wait for `check_lazy_txn_finished()` before deleting tablet data/metadata; if a test needs to bypass the check, please use a sync point rather than a mutable production config.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org