HappenLee commented on code in PR #68658:
URL: https://github.com/apache/doris/pull/68658#discussion_r4141516679
##########
fe/fe-core/src/main/java/org/apache/doris/common/profile/Profile.java:
##########
@@ -314,7 +314,7 @@ public synchronized void updateSummary(Map<String, String>
summaryInfo, boolean
long durationThreshold = executionProfiles.isEmpty()
? autoProfileDurationMs :
executionProfiles.size() * autoProfileDurationMs;
if (this.queryFinishTimestamp != Long.MAX_VALUE && durationMs
< durationThreshold) {
- ProfileManager.getInstance().removeProfile(this.getId());
+ ProfileManager.getInstance().removeProfile(this);
if (LOG.isDebugEnabled()) {
Review Comment:
Confirmed this retry cleanup gap locally on
`c588e48817809f120a4296a2a6d5d32615df8729`.
The change correctly cleans up the first short failed attempt, but a
subsequent attempt can still retain its `ExecutionProfile`:
1. With profiling enabled and a positive auto-profile threshold, a fast
retryable RPC failure during dispatch reaches `finalizeQuery()`.
`updateSummary(..., true, ...)` sets `isQueryFinished = true` and removes the
first execution profile.
2. `handleQueryWithRetry()` reuses that same `Profile`, changes the query
ID, and registers another execution profile.
3. Both summary updates for the retry return immediately at the
`isQueryFinished` guard, so this removal is never reached again.
`unregisterQuery()` only marks the execution profile finished.
The fallback collector is size-triggered (`size > 2 *
max_query_profile_num`, i.e. over 1000 entries by default), so a small number
of retained entries will not simply disappear after five seconds. This is a
pre-existing lifecycle gap left uncovered by this fix, rather than a new
regression introduced by the diff.
Please cover failed dispatch followed by a retry, and ensure cleanup applies
to the retry execution as well. The lifecycle needs to distinguish completion
of one attempt from completion of the whole statement.
**Validation:** ran the repository's `run-fe-ut.sh`: `ProfileTest` (15),
`AutoProfileTest` (1), and `ProfileManagerTest` (28) all passed. The additional
test below failed on the final assertion: the first execution was removed, but
the retry execution remained registered. It directly reproduces the Profile
lifecycle; it does not inject an actual BE RPC failure.
<details>
<summary>Minimal reproducer: ReviewProfileRetryTest.java</summary>
```java
package org.apache.doris.common.profile;
import org.apache.doris.common.util.DebugUtil;
import org.apache.doris.thrift.TUniqueId;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.parallel.ResourceLock;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
@ResourceLock("global")
public class ReviewProfileRetryTest {
@Test
public void retryExecutionMustBeReleasedAfterFinalization() {
ProfileManager manager = ProfileManager.getInstance();
Profile profile = new Profile(true, 1, 60_000);
profile.getSummaryProfile().setQueryBeginTime(System.currentTimeMillis());
TUniqueId firstId = new TUniqueId(68658, 1);
TUniqueId retryId = new TUniqueId(68658, 2);
Map<String, String> summary = new HashMap<>();
summary.put(SummaryProfile.PROFILE_ID, DebugUtil.printId(firstId));
try {
// registerQuery, dispatch failure, finalizeQuery: no running
summary was pushed.
ExecutionProfile first = new ExecutionProfile(firstId,
Collections.emptyList());
profile.addExecutionProfile(first);
manager.addExecutionProfile(first);
profile.updateSummary(summary, true, null);
manager.markExecutionProfileFinished(firstId);
Assertions.assertNull(manager.getExecutionProfile(firstId));
Assertions.assertTrue(profile.isQueryFinished);
// handleQueryWithRetry reuses Profile while replacing the query
ID and Coordinator.
ExecutionProfile retry = new ExecutionProfile(retryId,
Collections.emptyList());
profile.addExecutionProfile(retry);
manager.addExecutionProfile(retry);
summary.put(SummaryProfile.PROFILE_ID,
DebugUtil.printId(retryId));
profile.updateSummary(summary, false, null);
profile.updateSummary(summary, true, null);
manager.markExecutionProfileFinished(retryId);
Assertions.assertNull(manager.getExecutionProfile(retryId),
"The retry execution remains registered after
finalization");
} finally {
manager.removeProfile(profile);
}
}
}
```
Observed failure:
```text
The retry execution remains registered after finalization
expected: <null> but was: <DetailProfile(10c32-2): ...>
Tests run: 45, Failures: 1, Errors: 0, Skipped: 0
```
</details>
--
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]
