HappenLee commented on code in PR #68658:
URL: https://github.com/apache/doris/pull/68658#discussion_r4164901224
##########
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
+ // Profile decision, but discard the running history entry keyed
by the old ID.
+
ProfileManager.getInstance().removeProfileFromHistory(profile.getId());
+ } else {
+ // The final profile report occurs after BE returns the query
data. Update the profile
+ // before unregistering, or the instance profile can be lost.
Review Comment:
Please fix this before merging. I rechecked the current head
`fbb274bf92640a6757348741f72e4d876b906dc6`; the deferred retry finalization
still applies the existing multi-execution threshold to attempts of a single
query.
For example, with `enable_profile=true` and
`auto_profile_threshold_ms=5000`, a retryable dispatch failure after about 100
ms followed by a successful 6-second attempt leaves
`executionProfiles=[E1,E2]`. The inner retry does not reset the query start
time, so finalization compares approximately 6100 ms against `2 * 5000` and
removes the entire profile. Even the successful attempt alone exceeds the
configured threshold.
To be precise about provenance: the multiplication formula predates this PR.
This PR defers finalization until the last attempt, making that formula operate
on all retained retry attempts. The base already had a premature-finalization
defect, so this is not a claim that the base correctly retained the final
successful attempt.
A simpler fix for ordinary query retries would be to discard the previous
attempt's profile data before starting the next attempt and keep only the final
attempt's execution profile:
- Remove the previous history entry and its execution-profile registration
from ProfileManager.
- Also remove the previous execution profile from the shared Profile's
`executionProfiles` list. Calling the current `removeProfile(profile)` alone
does not clear that list.
- Keep the statement-level Profile open while retrying; finalize it only
after the last attempt, whether that attempt succeeds or fails.
- Keep Broker Load's multi-task behavior separate. For query retention,
preserve the statement's cumulative elapsed-time semantics rather than
multiplying the threshold by retry count.
This also avoids making the successful query lose its merged profile merely
because a failed attempt remains in the list.
Please add a regression unit test with a positive threshold and total
duration between one and two thresholds, asserting that the final profile is
retained and the abandoned attempt is removed from both the manager and the
execution list. The current tests using a zero threshold or a large threshold
do not cover this boundary.
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]
