You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "mymeiyi (via GitHub)" <gi...@apache.org> on 2026/05/15 06:55:37 UTC
[PR] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
mymeiyi opened a new pull request, #63286:
URL: https://github.com/apache/doris/pull/63286
(no comment)
--
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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #63286:
URL: https://github.com/apache/doris/pull/63286#discussion_r3246438135
##########
cloud/test/recycler_test.cpp:
##########
@@ -8693,4 +8693,76 @@ TEST(RecyclerTest, recycle_tablet_with_delete_file_failure) {
EXPECT_EQ(it->size(), 0) << "All recycle rowset keys should be deleted";
}
}
+TEST(RecyclerTest, enable_recycler_default_true) {
+ EXPECT_TRUE(config::enable_recycler);
+}
+
+TEST(RecyclerTest, enable_recycler_skip_instance_scanner) {
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+
+ bool old_val = config::enable_recycler;
+ config::enable_recycler = false;
+ DORIS_CLOUD_DEFER { config::enable_recycler = old_val; };
+
+ bool old_recycle_interval = config::recycle_interval_seconds;
Review Comment:
`recycle_interval_seconds` is an `int64_t` config, but this saves it into a `bool`. If the original value is the default `3600`, the deferred restore writes back `1`, which can contaminate later tests that rely on the configured recycle interval. Store the old value as `int64_t` before setting it to zero.
--
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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "gavinchou (via GitHub)" <gi...@apache.org>.
gavinchou merged PR #63286:
URL: https://github.com/apache/doris/pull/63286
--
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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #63286:
URL: https://github.com/apache/doris/pull/63286#discussion_r3247417279
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -249,26 +249,30 @@ void Recycler::instance_scanner_callback() {
std::this_thread::sleep_for(
std::chrono::seconds(config::recycler_sleep_before_scheduling_seconds));
while (!stopped()) {
- std::vector<InstanceInfoPB> instances;
- get_all_instances(txn_kv_.get(), instances);
- // TODO(plat1ko): delete job recycle kv of non-existent instances
- LOG(INFO) << "Recycler get instances: " << [&instances] {
- std::stringstream ss;
- for (auto& i : instances) ss << ' ' << i.instance_id();
- return ss.str();
- }();
- if (!instances.empty()) {
- // enqueue instances
- std::lock_guard lock(mtx_);
- for (auto& instance : instances) {
- if (instance_filter_.filter_out(instance.instance_id())) 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));
+ if (config::enable_recycler) {
+ std::vector<InstanceInfoPB> instances;
+ get_all_instances(txn_kv_.get(), instances);
+ // TODO(plat1ko): delete job recycle kv of non-existent instances
+ LOG(INFO) << "Recycler get instances: " << [&instances] {
+ std::stringstream ss;
+ for (auto& i : instances) ss << ' ' << i.instance_id();
+ return ss.str();
+ }();
+ if (!instances.empty()) {
+ // enqueue instances
Review Comment:
When `enable_recycler` is false, the scanner reaches this `wait_for` and sleeps for `recycle_interval_seconds` before checking the flag again. `update_config` only writes the config value; it does not notify `Recycler::notifier_`, so if an operator re-enables the recycler just after this wait starts, scanning can remain stopped until the default 3600s interval expires. For a dynamic enable/disable switch, please make the disabled state poll a short interval or add a config-change wakeup path so re-enabling resumes recycler scheduling promptly.
--
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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "mymeiyi (via GitHub)" <gi...@apache.org>.
mymeiyi commented on PR #63286:
URL: https://github.com/apache/doris/pull/63286#issuecomment-4458835723
/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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #63286:
URL: https://github.com/apache/doris/pull/63286#issuecomment-4457998650
# Cloud UT Coverage Report
Increment line coverage `100.00% (24/24)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/97274a603bcaad0affd24bee6e7521a6e29b13f5_97274a603bcaad0affd24bee6e7521a6e29b13f5_cloud/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/97274a603bcaad0affd24bee6e7521a6e29b13f5_97274a603bcaad0affd24bee6e7521a6e29b13f5_cloud/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 78.06% (1854/2375) |
| Line Coverage | 64.53% (33339/51662) |
| Region Coverage | 65.24% (16532/25341) |
| Branch Coverage | 55.75% (8838/15852) |
--
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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #63286:
URL: https://github.com/apache/doris/pull/63286#issuecomment-4457663544
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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #63286:
URL: https://github.com/apache/doris/pull/63286#discussion_r3246471833
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -298,6 +302,11 @@ void Recycler::recycle_callback() {
// skip instance in recycling
if (recycling_instance_map_.count(instance_id)) continue;
}
Review Comment:
This only prevents a dequeued instance from starting after the flag is already false. If `enable_recycler` is flipped to false while an `InstanceRecycler::do_recycle()` round is already running, that round keeps deleting data/KV and `lease_recycle_jobs()` continues renewing its job because neither path observes the new switch. For a dynamic recycler disable switch, that leaves the most important operational case uncovered: disabling recycler during a suspected bad recycle does not stop the in-flight recycle work. Please also stop/abort the entries in `recycling_instance_map_` when the flag becomes false, or make the active recycle/lease paths check the flag and stop promptly.
##########
cloud/test/recycler_test.cpp:
##########
@@ -8693,4 +8693,85 @@ TEST(RecyclerTest, recycle_tablet_with_delete_file_failure) {
EXPECT_EQ(it->size(), 0) << "All recycle rowset keys should be deleted";
}
}
+
+TEST(RecyclerTest, enable_recycler_default_true) {
+ EXPECT_TRUE(config::enable_recycler);
+}
+
+TEST(RecyclerTest, enable_recycler_skip_instance_scanner) {
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+
+ bool old_val = config::enable_recycler;
+ config::enable_recycler = false;
+ DORIS_CLOUD_DEFER {
+ config::enable_recycler = old_val;
+ };
+
+ int64_t old_recycle_interval = config::recycle_interval_seconds;
+ config::recycle_interval_seconds = 0;
+ DORIS_CLOUD_DEFER {
+ config::recycle_interval_seconds = old_recycle_interval;
+ };
+
+ int64_t old_sleep = config::recycler_sleep_before_scheduling_seconds;
+ config::recycler_sleep_before_scheduling_seconds = 0;
+ DORIS_CLOUD_DEFER {
+ config::recycler_sleep_before_scheduling_seconds = old_sleep;
+ };
+
+ Recycler recycler(txn_kv);
+ std::thread t([&]() { recycler.instance_scanner_callback(); });
+
+ // Let the callback complete one iteration:
+ // sleep(0) -> check enable_recycler (false, skip) -> wait_for(0, timeout)
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+
+ recycler.stopped_ = true;
+ recycler.notifier_.notify_all();
+ t.join();
+
+ EXPECT_TRUE(recycler.pending_instance_queue_.empty());
Review Comment:
This assertion does not prove that scanning was skipped. The test never writes any instance metadata into `txn_kv`, so `pending_instance_queue_` would remain empty even if the new `if (config::enable_recycler)` guard in `instance_scanner_callback()` were removed and `get_all_instances()` ran normally. Please seed an instance that would be enqueued when the flag is true, or otherwise assert that `get_all_instances()`/enqueue is not reached while disabled.
##########
cloud/test/recycler_test.cpp:
##########
@@ -8693,4 +8693,85 @@ TEST(RecyclerTest, recycle_tablet_with_delete_file_failure) {
EXPECT_EQ(it->size(), 0) << "All recycle rowset keys should be deleted";
}
}
+
+TEST(RecyclerTest, enable_recycler_default_true) {
+ EXPECT_TRUE(config::enable_recycler);
+}
+
+TEST(RecyclerTest, enable_recycler_skip_instance_scanner) {
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+
+ bool old_val = config::enable_recycler;
+ config::enable_recycler = false;
+ DORIS_CLOUD_DEFER {
+ config::enable_recycler = old_val;
+ };
+
+ int64_t old_recycle_interval = config::recycle_interval_seconds;
+ config::recycle_interval_seconds = 0;
+ DORIS_CLOUD_DEFER {
+ config::recycle_interval_seconds = old_recycle_interval;
+ };
+
+ int64_t old_sleep = config::recycler_sleep_before_scheduling_seconds;
+ config::recycler_sleep_before_scheduling_seconds = 0;
+ DORIS_CLOUD_DEFER {
+ config::recycler_sleep_before_scheduling_seconds = old_sleep;
+ };
+
+ Recycler recycler(txn_kv);
+ std::thread t([&]() { recycler.instance_scanner_callback(); });
+
+ // Let the callback complete one iteration:
+ // sleep(0) -> check enable_recycler (false, skip) -> wait_for(0, timeout)
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+
+ recycler.stopped_ = true;
+ recycler.notifier_.notify_all();
+ t.join();
+
+ EXPECT_TRUE(recycler.pending_instance_queue_.empty());
+}
+
+TEST(RecyclerTest, enable_recycler_skip_recycle_callback) {
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+
+ bool old_val = config::enable_recycler;
+ config::enable_recycler = false;
+ DORIS_CLOUD_DEFER {
+ config::enable_recycler = old_val;
+ };
+
+ Recycler recycler(txn_kv);
+
+ InstanceInfoPB instance;
+ instance.set_instance_id("test_instance");
+ recycler.pending_instance_queue_.push_back(instance);
+ recycler.pending_instance_set_.insert("test_instance");
+
+ std::thread t([&]() { recycler.recycle_callback(); });
+
+ // Wait until the callback has popped the instance from the queue.
+ // Can not wait on pending_instance_cond_ here because the callback does
+ // not notify after popping, which may cause a deadlock: both the main
+ // thread and the callback end up waiting on the same CV with different
+ // predicates and no one will wake them up.
+ while (true) {
+ {
+ std::lock_guard lock(recycler.mtx_);
+ if (recycler.pending_instance_queue_.empty()) break;
+ }
+ std::this_thread::yield();
+ }
+
+ recycler.stopped_ = true;
+ recycler.pending_instance_cond_.notify_all();
+ t.join();
+
+ EXPECT_TRUE(recycler.pending_instance_queue_.empty());
Review Comment:
These final-state assertions do not prove that the recycle callback skipped work because of `enable_recycler=false`. With the guard removed, the worker still pops the queue and can fail early on this mostly empty `InstanceInfoPB`, leaving the queue/set/map empty as asserted here. Please make the test distinguish the disabled path from an attempted recycle, for example by asserting no recycle job key is created or by using a valid instance plus instrumentation/sync point that would fail if `InstanceRecycler::init()` or `do_recycle()` is reached.
--
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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "mymeiyi (via GitHub)" <gi...@apache.org>.
mymeiyi commented on PR #63286:
URL: https://github.com/apache/doris/pull/63286#issuecomment-4457667312
/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] [improvement](cloud) Add enable_recycler config to skip recycler dynamically [doris]
Posted by "mymeiyi (via GitHub)" <gi...@apache.org>.
mymeiyi commented on PR #63286:
URL: https://github.com/apache/doris/pull/63286#issuecomment-4457740575
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