github-actions[bot] commented on code in PR #68658:
URL: https://github.com/apache/doris/pull/68658#discussion_r4143010094
##########
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());
Review Comment:
[P1] Keep the Profile open for the outer cloud replan retry too. A
post-registration dispatch error containing NO_SCAN_NODE_BACKEND_AVAILABLE_MSG
goes through the early throw in handleQueryWithRetry with willRetry=false, so
this branch marks the shared Profile finished. queryRetry then starts a new
query ID on the same StmtExecutor; both of its summary updates return at
Profile.isQueryFinished, and unregister only marks its ExecutionProfile
finished. That entry remains until size-triggered GC, while a retained history
entry still describes the failed ID. Carry the outer retry decision into
finalization or reset the Profile before the next attempt, and test a
dispatch-time replan error followed by success.
##########
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) {
Review Comment:
[P2] Do not let an abandoned dispatch keep the successful retry's profile
incomplete. Real ExecutionProfiles start with nonempty fragment IDs and zero
expected backend reports; if dispatch fails before fragments are sent,
isCompleted() stays false. Retaining that attempt here makes
Profile.shouldStoreToStorage wait the spill timeout and then mark the final
successful profile INCOMPLETE even when its own backend reports finished. The
new retry tests use empty fragment lists, for which isCompleted() is always
true. Exclude the abandoned attempt from completion accounting or mark it
terminal, and cover a nonempty failed attempt.
##########
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:
[P2] Render the successful retry's merged query profile. Keeping the failed
attempt here makes executionProfiles.size() equal two, while
Profile.getExecutionProfileContent only builds MergedProfile, fills
rowsProducedMap, and annotates PhysicalPlan actual rows when the size is one.
Thus a retained successful retry loses its normal merged runtime view and
actual row counts in both REST and stored profiles. Show the last successful
attempt's merged view while preserving earlier attempts separately, and assert
profile text in a retry test.
##########
fe/fe-core/src/main/java/org/apache/doris/common/profile/ProfileManager.java:
##########
@@ -798,32 +794,32 @@ protected void deleteOutdatedProfilesFromStorage() {
}
try {
- List<ProfileElement> queryIdToBeRemoved = Lists.newArrayList();
+ List<ProfileElement> profilesToBeRemoved = Lists.newArrayList();
readLock.lock();
try {
- queryIdToBeRemoved = getProfilesToBeRemoved();
+ profilesToBeRemoved = getProfilesToBeRemoved();
} finally {
readLock.unlock();
}
- if (queryIdToBeRemoved.isEmpty()) {
+ if (profilesToBeRemoved.isEmpty()) {
return;
}
// Archive or delete profiles based on configuration
if (Config.enable_profile_archive) {
// Move profiles to pending directory for archiving
- moveProfilesToArchivePending(queryIdToBeRemoved);
Review Comment:
[P2] Handle Broker Load IDs when pruning stored profiles. This is
pre-existing: a Broker Load profile uses its decimal job ID, but after deleting
or archiving the selected files this loop parses that ID as a TUniqueId on the
next line. The parser throws for a decimal ID, so the outer catch stops cleanup
and later selected profiles remain indexed although their files were already
removed or moved. Remove each attached execution by its own query ID, as memory
eviction does, and cover mixed query/load profiles.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1303,12 +1314,13 @@ private void handleQueryWithRetry(TUniqueId queryId)
throws Exception {
}
if (i != retryTime - 1 && isNeedRetry &&
context.getProtocolAdapter().canRetryQuery(context)) {
LOG.warn("retry {} times. stmt: {}", (i + 1),
parsedStmt.getOrigStmt().originStmt);
+ willRetry = true;
} else {
throw e;
}
} finally {
Review Comment:
[P2] Clean the failed Flight SQL attempt on an outer cloud replan retry.
This is pre-existing: Flight beforeQuery switches results to the backends
before registration, so a post-registration replan error makes
isReturnResultFromLocal false and skips this finalization call. The failed
attempt never reaches deferForArrowFlight, while beforeAttempt and request
close act on the later query ID; its QeProcessor registration, execution
profile, and finish callbacks remain. This differs from the local-result outer
retry, which finalizes too early. Unregister the abandoned ID before replanning
and cover this Flight path.
##########
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:
[P2] Apply the configured query duration threshold after the final retry
without multiplying it by retry count. This deferred finish keeps two
ExecutionProfiles; Profile.updateSummary compares a 12-second query (11-second
failed dispatch plus 1-second successful retry) against 2 *
auto_profile_threshold_ms. At a 10-second setting the new path drops the whole
profile, whereas the base retained a profile after the first 11-second attempt.
Keep any Broker Load per-task policy separate and test a retry whose total
falls between one and two thresholds.
--
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]
