HappenLee commented on code in PR #68658:
URL: https://github.com/apache/doris/pull/68658#discussion_r4164952046
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1141,10 +1142,19 @@ private void parseByNereids() {
}
public void finalizeQuery() {
- // The final profile report occurs after be returns the query data,
and the profile cannot be
- // received after unregisterQuery(), causing the instance profile to
be lost, so we should wait
- // for the profile before unregisterQuery().
- updateProfile(true);
+ finalizeQuery(false);
+ }
+
+ void finalizeQuery(boolean willRetry) {
+ if (willRetry) {
+ // The next attempt uses a new query ID. Keep its execution
profiles for the final
Review Comment:
Please fix this before merging as well. At the current head
`fbb274bf92640a6757348741f72e4d876b906dc6`, retaining failed attempts in the
shared Profile prevents a successful retry from producing its normal merged
profile.
Example: enable profiling and set `auto_profile_threshold_ms=0` to exclude
the threshold issue. For an ordinary query such as `SELECT SUM(v) FROM t`, let
the first dispatch fail with a retryable RPC error and the second attempt
succeed with complete reports from two BEs. The shared list is then
`executionProfiles=[E1_failed, E2_successful]`.
`Profile.getExecutionProfileContent()` only calls
`getAggregatedFragmentsProfile(planNodeMap)` when `executionProfiles.size() ==
1`. With two attempts, it skips:
- Building and printing `MergedProfile` for the successful execution.
- Populating `rowsProducedMap` from that merged execution.
- Calling `updateActualRowCountOnPhysicalPlan()` to annotate actual rows
through this path.
The raw attempt profiles are still printed and the SQL result is unaffected,
but the successful execution loses its merged diagnostic view even when all of
its BE reports are complete.
The size-one restriction already exists in the base; the retry-retention
design needs to accommodate it. This is not a claim that the base's
premature-finalization behavior correctly handled retries.
For ordinary query retries, the simpler fix discussed in the threshold
thread is to discard the previous attempt's profile data before registering the
next one, leaving only the final attempt in the execution list. Cleanup must
remove both the manager registrations/history and the old list entry, while
keeping the statement-level Profile open until the last attempt finishes.
Preserve Broker Load's multi-task behavior. If retaining failed attempts is
required, select the final attempt explicitly for the merged view and display
earlier attempts separately; do not combine failed and successful executions'
counters.
Please add a test with nonempty fragment profiles and backend reports that
renders the retained profile after a successful retry, asserts that
`MergedProfile` is present, and verifies actual-row propagation for a known
plan node. Merely checking map membership or using empty fragment lists does
not cover this behavior.
Validation for this follow-up: static code-path review only; no additional
tests were run.
--
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]
