github-actions[bot] commented on code in PR #67053:
URL: https://github.com/apache/doris/pull/67053#discussion_r3840684408
##########
be/src/common/daemon.cpp:
##########
@@ -454,9 +454,13 @@ void Daemon::calculate_metrics_thread() {
}
void Daemon::report_runtime_query_statistics_thread() {
- while (!_stop_background_threads_latch.wait_for(
-
std::chrono::milliseconds(config::report_query_statistics_interval_ms))) {
-
ExecEnv::GetInstance()->runtime_query_statistics_mgr()->report_runtime_query_statistics();
+ auto* statistics_mgr =
ExecEnv::GetInstance()->runtime_query_statistics_mgr();
+ while (_stop_background_threads_latch.count() != 0) {
+ statistics_mgr->wait_for_runtime_query_statistics_report(
Review Comment:
This removes the old 3-second upper bound on full reports. A LOAD completion
arriving during a report leaves the bit set, so the next wait returns
immediately; each pass then exclusively walks and serializes every retained
query and synchronously calls every FE. Under sustained completions the
reporter can run continuously, or make full-report QPS track LOAD QPS. Please
add a bounded debounce within the 5-second audit budget or report only dirty
completed IDs while retaining periodic full reconciliation.
##########
be/src/runtime/query_context.cpp:
##########
@@ -248,6 +248,13 @@ QueryContext::~QueryContext() {
PrettyPrinter::print_bytes(query_mem_tracker()->peak_consumption()));
}
_resource_ctx->task_controller()->finish();
+#ifndef BE_TEST
+ if (_resource_ctx->task_controller()->query_type() == TQueryType::LOAD) {
Review Comment:
`QueryContext` destruction is not a reliable DML completion boundary in
either direction. With a nonempty runtime-filter merge entity,
`_query_ctx_map_delay_delete` strongly retains the context after successful
fragment completion, so this branch can miss FE's 5-second audit window.
Conversely receiver `LoadChannel`/`LoadStream` objects retain only the shared
`ResourceContext`; remote sender and final flush work can outlive QueryContext
and raise its memory peak after this report sends `query_finished=true` and
erases the entry. Move finish/wakeup to an explicit
all-ResourceContext-producers-complete edge, and test both an INSERT SELECT
runtime-filter join and a multi-sender receiver whose local fragments finish
first.
##########
be/src/common/daemon.cpp:
##########
@@ -454,9 +454,13 @@ void Daemon::calculate_metrics_thread() {
}
void Daemon::report_runtime_query_statistics_thread() {
- while (!_stop_background_threads_latch.wait_for(
-
std::chrono::milliseconds(config::report_query_statistics_interval_ms))) {
-
ExecEnv::GetInstance()->runtime_query_statistics_mgr()->report_runtime_query_statistics();
+ auto* statistics_mgr =
ExecEnv::GetInstance()->runtime_query_statistics_mgr();
+ while (_stop_background_threads_latch.count() != 0) {
+ statistics_mgr->wait_for_runtime_query_statistics_report(
+
std::chrono::milliseconds(config::report_query_statistics_interval_ms));
+ if (_stop_background_threads_latch.count() != 0) {
+ statistics_mgr->report_runtime_query_statistics();
Review Comment:
Waking this single thread does not cover the RPC-delay case in the PR
description. `report_runtime_query_statistics()` visits FE groups serially and
each call can use the default 60-second Thrift timeout, while FE emits the
audit event after 5 seconds. If the thread is blocked on unavailable FE A when
a LOAD for healthy FE B finishes, B's bit remains set but B cannot even be
snapshotted or sent before its audit deadline. Please isolate completion/per-FE
reporting from a slow FE with a deadline below the audit budget, and test one
blocked FE alongside one healthy completion.
##########
be/src/runtime/query_context.cpp:
##########
@@ -248,6 +248,13 @@ QueryContext::~QueryContext() {
PrettyPrinter::print_bytes(query_mem_tracker()->peak_consumption()));
}
_resource_ctx->task_controller()->finish();
+#ifndef BE_TEST
+ if (_resource_ctx->task_controller()->query_type() == TQueryType::LOAD) {
+ // Final DML counters must reach FE before its audit timeout. Periodic
reporting remains
+ // the retry path.
+
_exec_env->runtime_query_statistics_mgr()->trigger_runtime_query_statistics_report();
Review Comment:
This completion timing collides with the documented same-query-ID
group-commit path. The reporter can snapshot finished context A, release the
map lock for its RPC, let context B overwrite the same ID, and then erase B by
`query_id` when A's RPC succeeds; if B replaces A before the snapshot, A is
lost instead. Carry the snapshotted `ResourceContext` identity/generation
through cleanup, erase only if the current entry is still A, and preserve
completed generations replaced before snapshot.
##########
be/src/runtime/query_context.cpp:
##########
@@ -248,6 +248,13 @@ QueryContext::~QueryContext() {
PrettyPrinter::print_bytes(query_mem_tracker()->peak_consumption()));
}
_resource_ctx->task_controller()->finish();
Review Comment:
A report already running for another completion can interleave inside this
`finish()`: `TaskController::finish()` stores `is_finished_=true` before
`finish_time_`. The reporter can observe true with timestamp 0, classify the
task as past the 30-second reserve timeout, and erase it even when its RPC
fails; this task's queued trigger then has no retry state. Publish the
timestamp before release-publishing the finished flag, acquire-read that flag,
and add a deterministic interleaving test.
##########
be/test/runtime/runtime_query_statistics_mgr_test.cpp:
##########
@@ -0,0 +1,45 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "runtime/runtime_query_statistics_mgr.h"
+
+#include <gtest/gtest.h>
+
+#include <chrono>
+#include <future>
+
+namespace doris {
+
+TEST(RuntimeQueryStatisticsMgrTest,
QueryFinishTriggerWakesReporterAndCoalesces) {
Review Comment:
This exercises only the standalone notification bit. BE unit tests define
`BE_TEST`, which compiles the production QueryContext registration/trigger path
out, and the fixture never runs the daemon, report RPC, cleanup, or lifecycle
boundary. It therefore cannot detect delayed or premature QueryContext
destruction, a blocked FE, same-ID replacement, finish publication, or
unbounded cadence. The initial 10-ms future check also does not prove the
worker reached the wait before notification. Please add production-path tests
through an injectable reporter/daemon boundary.
--
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]
