github-actions[bot] commented on code in PR #60567:
URL: https://github.com/apache/doris/pull/60567#discussion_r3124782807
##########
be/src/runtime/workload_management/query_task_controller.cpp:
##########
@@ -212,4 +212,22 @@ std::vector<PipelineTask*>
QueryTaskController::get_revocable_tasks() {
return tasks;
}
+int QueryTaskController::get_total_task_num() const {
+ // Read task-level total counter from query context for external
statistics reporting.
+ auto query_ctx = query_ctx_.lock();
+ if (query_ctx == nullptr) {
+ return 0;
Review Comment:
`RuntimeQueryStatisticsMgr` intentionally keeps `ResourceContext` alive
after query end so it can send one last `TQueryStatisticsResult`, but
`QueryContext` is already gone by then: `task_controller()->finish()` is called
in `QueryContext::~QueryContext()`, and these accessors only look through
`query_ctx_.lock()`. Once the weak pointer can no longer lock, the final report
for a finished query emits `0` for both `total_tasks_num` and
`finished_tasks_num`, overwriting the last correct snapshot in FE. That makes a
completed query show `0/0` progress in `show current_queries`/audit paths. The
counters need to survive independently of `QueryContext` on the final report
path instead of being reconstructed through a `weak_ptr`.
##########
be/src/exec/pipeline/pipeline_fragment_context.cpp:
##########
@@ -602,6 +602,8 @@ Status
PipelineFragmentContext::_build_pipeline_tasks(ThreadPool* thread_pool) {
}
_pipeline_parent_map.clear();
_op_id_to_shared_state.clear();
+ // Record task cardinality once when this fragment context finishes task
initialization.
+
_query_ctx->add_total_task_num(_total_tasks.load(std::memory_order_relaxed));
Review Comment:
`_total_tasks` is still mutable after this point: in `submit()` we shrink it
to `submit_tasks` if scheduler submission fails. Because the query-wide counter
is only ever incremented here, a partial submit leaves
`QueryContext::_total_task_num` larger than the number of tasks that can
actually finish, so progress never reaches 100% on the failure path. The
previous implementation read `_total_tasks` after submit adjustments; this
atomicized version needs the same correction (for example, add the total only
after successful submit, or subtract the delta on submit failure).
##########
fe/fe-core/src/main/java/org/apache/doris/common/proc/ProcService.java:
##########
@@ -49,7 +49,7 @@ private ProcService() {
root.register("trash", new TrashProcDir());
root.register("monitor", new MonitorProcDir());
root.register("current_queries", new CurrentQueryStatisticsProcDir());
- root.register("current_query_stmts", new
CurrentQueryStatementsProcNode());
+ root.register("current_query_stmts", new
CurrentQueryStatisticsProcDir());
Review Comment:
`/current_query_stmts` used to expose the narrow statement-only schema from
`CurrentQueryStatementsProcNode`
(`QueryId/ConnectionId/Catalog/Database/User/ExecTime/SqlHash/Statement`).
Repointing it to `CurrentQueryStatisticsProcDir` silently changes both the
columns and semantics for existing consumers. If the new statistics view should
also be available there, we still need a compatibility node (or a separate
endpoint) instead of replacing the old proc contract.
##########
fe/fe-core/src/main/java/org/apache/doris/common/proc/CurrentQueryStatisticsProcDir.java:
##########
@@ -47,15 +56,7 @@ public boolean register(String name, ProcNodeInterface node)
{
@Override
public ProcNodeInterface lookup(String name) throws AnalysisException {
- if (Strings.isNullOrEmpty(name)) {
- return null;
- }
- final Map<String, QueryStatisticsItem> statistic =
QeProcessorImpl.INSTANCE.getQueryStatistics();
- final QueryStatisticsItem item = statistic.get(name);
- if (item == null) {
- throw new AnalysisException(name + " doesn't exist.");
- }
- return new CurrentQuerySqlProcDir(item);
Review Comment:
This turns every existing drill-down proc path into an error. Before this
change, `SHOW PROC '/current_queries/<query_id>'` returned the SQL text and
`SHOW PROC '/current_queries/<query_id>/fragments'` returned per-fragment stats
via `CurrentQuerySqlProcDir` / `CurrentQueryFragmentProcNode`. The PR goal is
to add progress columns to the top-level table, not remove those public proc
subpaths, so this is a user-visible compatibility regression.
--
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]
