github-actions[bot] commented on code in PR #67006:
URL: https://github.com/apache/doris/pull/67006#discussion_r3829293451
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -1189,10 +1237,15 @@ Status FragmentMgr::merge_filter(const
PMergeFilterRequest* request,
query_id.__set_lo(queryid.lo);
if (auto q_ctx = get_query_ctx(query_id)) {
SCOPED_ATTACH_TASK(q_ctx.get());
- if (!q_ctx->get_merge_controller_handler()) {
+ auto handler = q_ctx->get_merge_controller_handler();
+ if (!handler) {
return Status::InternalError("Merge filter failed: Merge
controller handler is null");
}
- return q_ctx->get_merge_controller_handler()->merge(q_ctx, request,
attach_data);
+ auto status = handler->merge(q_ctx, request, attach_data);
+ if (handler->all_filters_published()) {
Review Comment:
This remains reproducible when the merge backend hosts no recursive-side
child fragment. FE chooses `runtime_filter_merge_addr` from topmost-plan
workers, while only recursive child fragments receive `need_notify_close` and
populate each BE's local `_rerunnable_params_map`. With the top/root and merge
controller on BE-M and all recursive children on BE-C, BE-M's map is empty
after the first round, so this code erases delayed ownership exactly as before;
reset never reinserts it, and a final merge arriving after root EOS returns
EOF. The new test manually inserts a rerunnable entry on the merge backend and
does not cover this valid placement. Please make generation/finality explicit
on the merge backend (or use an explicit query-terminal signal) and add a
disjoint-worker test.
##########
be/src/exec/runtime_filter/runtime_filter_mgr.cpp:
##########
@@ -434,6 +437,16 @@ Status
RuntimeFilterMergeControllerEntity::init(std::shared_ptr<QueryContext> qu
return Status::OK();
}
+bool RuntimeFilterMergeControllerEntity::all_filters_published() {
+ SharedLockGuard guard(_filter_map_mutex);
+ for (const auto& filter : _filter_map) {
+ if (!filter.second.done.load()) {
Review Comment:
[P1] Release retained contexts when an ignored merge RPC never arrives
With `ignore_runtime_filter_error=true`, `_push_to_remote()` gives
`HandleErrorBrpcCallback` an empty QueryContext, so a dropped, timed-out, or
rejected `merge_filter` RPC only logs and the query is allowed to complete. The
merge backend never sees that producer, this `done` bit remains false, and
every new release path stays gated by `all_filters_published()`; the delay map
itself then prevents `QueryContext::~QueryContext()` from providing cleanup.
Since leakage detection is disabled by default and execution timeout may be
nonpositive, repeated ignored RF failures can still retain full completed-query
contexts indefinitely. Please add a query-terminal or bounded-grace cleanup
independent of receiving every merge RPC, and cover the ignored-failure path.
##########
be/src/runtime/query_context.cpp:
##########
@@ -344,6 +344,10 @@ void QueryContext::cancel(Status new_status, int
fragment_id) {
set_ready_to_execute(new_status);
cancel_all_pipeline_context(new_status, fragment_id);
+ // fragment_mgr is nullptr in unit tests that create QueryContext in
isolation.
+ if (_exec_env->fragment_mgr()) {
+ _exec_env->fragment_mgr()->remove_query_context(_query_id);
Review Comment:
[P1] Prevent post-cancel rerunnable publication from escaping cleanup
This cleanup is only executed by the first successful status transition, but
fragment publication is not serialized with it. A recursive PFC can pass
`PipelineTask::prepare()`'s final cancellation check, then be cancelled here
while it is not yet present in `_pipeline_map` or `_rerunnable_params_map`;
`exec_plan_fragment()` can subsequently insert the cancelled PFC plus a new
`RerunableFragmentInfo` strong reference and submit it. The scheduler later
calls `QueryContext::cancel()` again, but the early return on the
already-terminal status skips this removal, so even when the PFC self-removes
the new rerunnable entry keeps the QueryContext indefinitely (and a FINAL_CLOSE
that ran before publication is not retried). Please serialize cancellation with
publication or add a terminal tombstone/recheck that rejects and drains
post-cancel inserts, and cover this ordering with a barrier-based test.
--
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]
