github-actions[bot] commented on code in PR #70218:
URL: https://github.com/apache/doris/pull/70218#discussion_r4476514236
##########
be/src/agent/task_worker_pool.cpp:
##########
@@ -2594,6 +2594,7 @@ void calc_delete_bitmap_callback(CloudStorageEngine&
engine, const TAgentTaskReq
Review Comment:
[P1] Removing this signature makes FE resend the notification forever.
`sendMakeCloudTmpRsVisibleTasks` adds `MakeCloudTmpRsVisibleTask` to
`AgentTaskQueue`; this type is resendable, and `ReportHandler.taskReport`
resubmits queue entries absent from BE-reported signatures. This callback never
sends `finish_task` or removes the FE queue entry, so the scope-exit cleanup
makes each normal notification disappear from BE reports and get resent on
every report cycle. Complete/remove the FE task, or make it non-resend, before
clearing the BE signature.
##########
be/test/agent/task_worker_pool_test.cpp:
##########
@@ -106,6 +109,41 @@ TEST(TaskWorkerPoolTest, PreSubmitCallbackWithDedup) {
EXPECT_EQ(callback_count.load(), 1);
}
+class CloudCommittedRowsetTaskTest : public testing::TestWithParam<bool> {};
+
+TEST_P(CloudCommittedRowsetTaskTest, ReleasesSignatureAfterCallback) {
+ const bool original_enabled = config::enable_cloud_make_rs_visible_on_be;
+ Defer restore_config = [&] { config::enable_cloud_make_rs_visible_on_be =
original_enabled; };
+ config::enable_cloud_make_rs_visible_on_be = GetParam();
Review Comment:
[P1] The new test cannot compile because it reaches into a protected member.
`TaskWorkerPool::_thread_pool` is declared under `protected:` in
`task_worker_pool.h`, but this free `TEST_P` body calls
`workers._thread_pool->wait()` at both waits. Use a public wait/test helper
instead of accessing the member directly.
##########
be/src/common/config.cpp:
##########
@@ -2148,9 +2148,9 @@ DEFINE_mInt32(concurrency_stats_dump_interval_ms, "100");
DEFINE_Validator(concurrency_stats_dump_interval_ms,
Review Comment:
[P1] Defaulting this flag to false lets MoW partial updates snapshot stale
local rowsets. `CloudRowsetBuilder::init` now skips
`CloudTablet::sync_rowsets()` before `init_mow_context`, which probes only the
local snapshot. If a prior commit exists in Meta Service but a best-effort
notification was missed (or during a BE-first rollout), an existing key is
classified as new; `ERROR` policy fails the load, while append policy can fill
omitted columns with defaults/nulls instead of historical values. Keep
synchronization enabled until a freshness fallback is in place, or sync
whenever the snapshot is not known current.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3759,7 +3759,7 @@ public void handle(Field field, String value) throws
Exception {
public static boolean enable_tso_forward_when_counter_full = true;
Review Comment:
[P2] Enabling this FE default sends visibility tasks to old BEs whose
`enable_cloud_make_rs_visible_on_be` default is still false. Their callback
returns after `_submit_task` has inserted the signature into
`s_task_signatures`, so every committed transaction leaves a signature in the
old BE's set and report payload indefinitely during a rolling upgrade. Gate
this default on BE capability/upgrade, or make the old early-return path clean
up.
--
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]
