HappenLee commented on code in PR #68658:
URL: https://github.com/apache/doris/pull/68658#discussion_r4214894151


##########
fe/fe-core/src/main/java/org/apache/doris/common/profile/Profile.java:
##########
@@ -281,6 +281,11 @@ public List<ExecutionProfile> getExecutionProfiles() {
         return this.executionProfiles;
     }
 
+    public synchronized void clearExecutionProfiles() {

Review Comment:
   Rechecked this on the current head `b730ef741df0`. The phase timestamps 
still span different attempts: `queryPlanFinishTime` keeps its first value, 
while the retry overwrites `queryScheduleFinishTime`. As a result, `Schedule 
Time` and `Assign Fragment Time` can include the failed attempt, retry backoff, 
and replanning time.
   
   Could we expose the failed-attempt count in the Profile so that this retry 
history is visible? Please distinguish it from the retry count: one initial 
failure followed by a failed retry means two failed attempts, but only one 
retry.
   
   The count would help explain the output, but the phase-time calculation also 
needs a consistent scope. My preference is statement-level `Total Time` 
including retries, with phase timings describing the final attempt and their 
timestamps reset at the appropriate retry boundary. Otherwise, even with a 
failure-count label, time spent waiting or replanning would still appear as 
scheduling time.



##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -704,6 +724,10 @@ public void queryRetry(TUniqueId queryId) throws Exception 
{
                         DebugUtil.printId(queryId), randomMillis);
                 Thread.sleep(randomMillis);
                 context.getState().reset();
+                // The terminal branches above have thrown; another attempt 
will now run.
+                if (finishProfileInQueryRetry) {
+                    profile.clearExecutionProfiles();

Review Comment:
   No change is required for this issue in this PR. Let's keep the 
execution-profile leak fix focused; the statement-level cumulative duration and 
retention-threshold semantics can be considered separately. I am not treating 
this issue as a merge blocker for this PR.



-- 
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]

Reply via email to