Repository navigation
[fix](profile) Fix execution profile leak when dispatch fails before profile registration - #68658
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static-only review complete for head c588e48817809f120a4296a2a6d5d32615df8729. Request changes: one new P1 inline finding. The new cleanup removes execution profiles for a single short query even when no historical summary was inserted, but a dispatch failure followed by a query retry can still retain the retry's execution profile. No existing P0/P1 inline comment was present or independently confirmed. Two review rounds converged, including normal whole-PR and focused lifecycle reviews; the final changed-file sweep found no other substantiated new issue.
Checkpoint conclusions:
- Goal and proof: The direct absent-summary case is covered by a new unit test, but the intended memory-leak fix is incomplete for dispatch retries. The tests do not exercise failed dispatch followed by retry.
- Scope and reuse: The map and API renames are otherwise behavior-preserving:
Profile.getId()delegates toSummaryProfile.getProfileId(). The cleanup API change is focused; its caller's attempt lifecycle needs correction. - Concurrency and locking: The two manager maps are modified under the same write lock. BE profile reports run asynchronously and can hold a captured execution-profile object; no new lock-order issue, metadata-lock IO, or heavy work under a changed lock was found. The P1 is a sequential retry lifecycle failure.
- Lifecycle:
StmtExecutorreuses oneProfileacross attempts. Its first terminal update setsisQueryFinished; later attempts register new execution IDs but cannot reach terminal cleanup. Successful Broker Load finalization follows its task callbacks, with no additional add/remove race established. - Configuration: No new setting or dynamic-change obligation is introduced. The P1 occurs with a positive existing auto-profile threshold and the configured query retry path;
max_query_retry_timedefaults to 3. - Compatibility: No FE/BE wire value, storage format, function symbol, or rolling-upgrade behavior changed.
- Parallel paths: Normal MySQL, collected internal-query, deferred Arrow Flight, and successful Broker Load paths were traced. The retry path is the one unresolved defect; cloud replan and insert retries share that root cause rather than requiring duplicate comments.
- Conditional checks: The changed short-duration branch removes the caller's attached execution IDs. Its existing
isQueryFinishedguard prevents this on later attempts. Other renamed conditions retain their prior meaning. - Tests and results: New FE unit cases check absent/present summary entries and multiple execution IDs. They do not cover a real retry or Broker Load cancellation. No test result files changed. Builds and tests were prohibited by the review instructions and were not run; conclusions are static only.
- Observability: Existing debug logs show missing/removed execution profiles. No distinct logging or metric defect was established; logging would not release the retained retry entry.
- Transactions, persistence, and data writes: The PR changes no transaction, EditLog, table-data write, or persistent profile format path; existing profile storage and Broker Load commit ordering were checked without a new defect.
- FE/BE variables and performance: No new transmitted variable exists. Cleanup is proportional to the attached execution profiles and no additional hot-path anti-pattern was substantiated.
- Authorization and other issues: The REST call still applies the same non-admin profile owner check. No other candidate remains unresolved after duplicate and risk review.
User review focus: none was supplied. A separate pre-existing Broker Load cancellation retention path was noted during review; it does not arise from this query-dispatch change and is not duplicated as an inline finding.
| if (this.queryFinishTimestamp != Long.MAX_VALUE && durationMs < durationThreshold) { | ||
| ProfileManager.getInstance().removeProfile(this.getId()); | ||
| ProfileManager.getInstance().removeProfile(this); | ||
| if (LOG.isDebugEnabled()) { |
There was a problem hiding this comment.
[P1] Clean execution profiles for every dispatch retry. handleQueryWithRetry calls finalizeQuery() after each attempt, but it reuses this Profile when an RpcException from coordBase.exec() triggers a retry with a new query ID. The first short attempt marks isQueryFinished and reaches this removal; the retry then registers another ExecutionProfile, while both later updateSummary calls return immediately at the finished guard. unregisterQuery only marks that second entry finished, so it remains in queryIdToExecutionProfiles until the size-triggered fallback GC. Make each attempt eligible for cleanup and cover a failed-dispatch-then-retry case that checks both IDs.
There was a problem hiding this 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:
- With profiling enabled and a positive auto-profile threshold, a fast retryable RPC failure during dispatch reaches
finalizeQuery().updateSummary(..., true, ...)setsisQueryFinished = trueand removes the first execution profile. handleQueryWithRetry()reuses that sameProfile, changes the query ID, and registers another execution profile.- Both summary updates for the retry return immediately at the
isQueryFinishedguard, 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.
Minimal reproducer: ReviewProfileRetryTest.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:
The retry execution remains registered after finalization
expected: <null> but was: <DetailProfile(10c32-2): ...>
Tests run: 45, Failures: 1, Errors: 0, Skipped: 0
…le registration ### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: When query dispatch fails before the first summary update, the execution profile is registered but the historical profile is not. Short-query cleanup previously removed execution profiles only when it found a historical profile, leaving the execution profile in FE memory. Use the current Profile to remove its execution profiles regardless of whether its summary was pushed. This also handles profiles containing multiple executions. ### Release note Prevent FE profile memory growth when short profiled queries fail during dispatch. ### Check List (For Author) - Test: Unit Test (ProfileTest and ProfileManagerTest, 43 passed); FE build and Checkstyle passed. - Behavior changed: Yes. Short failed queries release their execution profiles. - Does this need documentation: No.
1e943bb to
fbb274b
Compare
|
/review |
There was a problem hiding this comment.
Complete static review
Reviewed PR #68658 at head fbb274bf92640a6757348741f72e4d876b906dc6 against base 3e3bacd60e1fca0aef2ce7a3c83516d45521973b. All eight changed files and related retry, profile-manager, Broker Load, REST, storage, and protocol paths were covered in three convergence rounds. The review found one P1 and five P2 issues, all reported inline. M5 and M6 are explicitly labeled pre-existing defects in touched or parallel paths.
The patch repairs the original short failed-dispatch cleanup gap when no history entry was published: removeProfile(Profile) now deletes attached execution IDs directly. It also prevents the earlier inner RpcException retry from finishing the shared Profile after its first attempt. Existing inline P1 comment 4140101788 described that inner retry and no longer applies to this head, so there is no still-applicable existing blocking comment ID. The separate outer cloud replan path remains affected (new P1 inline). There was no additional user-provided focus.
Critical checkpoints
- Goal and tests: The new
ProfileTestcases verify map cleanup with and without a history entry and with multiple execution IDs. TheStmtExecutorTestcases manually finalize mocked attempts and use empty fragment lists; they do not prove real dispatch/replan behavior, nonempty-fragment completion, configured threshold retention, or rendered profile content. The goal is only partially achieved because the inline findings remain. - Scope and lifecycle: The
ProfileManagermap rename follows the sameSummaryProfileID;QueryProfileActionchanges a method name only. The central lifecycle is registration before dispatch, running summary after dispatch, and terminal update before unregister. Inner retry now defers terminal update, but outer replan still finishes too early (P1), and abandoned attempts affect completion, retention, and rendering (P2 findings). - Concurrency and locking:
ProfileManagermap operations use its read/write lock;Profile.updateSummaryis synchronized. Late BE reports can hold an already retrieved execution object after map removal, but no new lock-order or report-thread race was substantiated. No cross-translation-unit initialization is involved. - Configuration and parallel paths: No new configuration, transmitted FE/BE variable, protocol symbol, or format was added. The existing
auto_profile_threshold_msis applied incorrectly after a retry. MySQL local-result, Flight backend-result, outer replan, Broker Load, memory eviction, and storage paths were traced; the pre-existing Flight orphan registration and Broker Load retention defect are inline with their provenance stated. - Tests, results, and observability: No result file changed. The review was static only: no builds or tests were run, as required by the review prompt; author or prior-thread test claims were not treated as independent validation. Existing logging and profile status expose some failures, but a successful retry can show an incomplete profile or lose its merged runtime view and actual row counts.
- Persistence, data, and performance: No EditLog, transaction, table-data write, or crash-recovery protocol changes were made. Profile file retention is covered by the Broker Load inline finding. The outer retry can retain execution state; abandoned attempts can delay spill and suppress a profile that exceeds its configured threshold. No other material hot-path cost was substantiated.
Disposition: Request changes for the P1 outer-replan lifecycle issue. The remaining accepted points are P2. No unresolved candidate remained after the final changed-file sweep.
|
/review |
There was a problem hiding this comment.
Static review of PR #68658 at head 1f6e14eaab459f045d2f0cd552be98618e560472.
The change fixes the described pre-summary execution-profile leak by removing attached execution IDs even when no profile-history entry exists. The inner retry, outer replan, Flight cleanup, and Broker Load pruning paths were traced. Three distinct P2 profile-correctness issues remain in the inline comments: outer retry duration/retention, stale physical-relation statistics after a post-dispatch replan, and cross-attempt phase timing.
Review completion: The bounded three rounds were performed. A new valuable finding arose in round 3, so convergence is incomplete under this review's instruction set. Every discovered candidate was independently validated, deduplicated, and given a disposition. No still-applicable P0/P1 inline finding was confirmed; the earlier P1 retry comments are addressed on this head and are not reposted.
Critical checkpoints
- Goal and proof: The direct cleanup and added FE unit tests cover a missing history entry, retained entry, multiple executions, inner retries, merged rendering, and mocked cloud outcomes. They do not cover the three inline scenarios; the cloud test uses a zero duration threshold and a generic mocked planner.
- Scope: Production changes are confined to profile ownership, retry finalization, identity naming, and the REST auth method rename. The manager rename and tests remain related to that work.
- Concurrency: BE reports and finish callbacks can run asynchronously. Manager map access uses its read/write lock; abandoned IDs are removed before the next attempt. No distinct lock-order or callback leak was substantiated. Profile lifecycle across query IDs is the material risk identified below.
- Configuration: No new setting was added. The existing
auto_profile_threshold_mspolicy is misapplied after an outer replan as described inline; the separate per-attempt timeout clock should be preserved. - Compatibility and parallel paths: No FE/BE wire, storage-format, or rolling-upgrade change was found. MySQL, Flight, direct execution, inner retry, outer cloud retry, and Broker Load paths were checked. The REST ownership check still compares the indexed profile owner under the manager lock.
- Conditions and errors: The inner retry gate and outer
UserExceptionreplan gate were traced through terminal and successful outcomes. Profile-update errors retain the existing logged-and-suppressed boundary; no additional error propagation bug was substantiated. - Tests and results: The changed FE tests were inspected for their assertions, including archive/delete and merged-profile content. No build, test, or regression suite was run because this review is static-only.
- Observability and performance: Existing logs identify failed profile updates. The inline issues affect the accuracy or retention of diagnostic profiles; no separate hot-path performance problem was substantiated.
- Persistence, writes, and protocol state: Stored profiles reuse the same rendering path, so the stale relation appendix can persist. Broker Load cleanup now removes attached execution IDs. No transaction data-write path or new FE-to-BE variable is changed.
- Other core invariants: Data visibility, partition versions, delete bitmaps, BE memory tracking, and static initialization are outside the changed path. No additional issue was substantiated.
No additional user review focus was supplied. This review used static code and test inspection only.
|
run buildall |
TPC-H: Total hot run time: 27915 ms |
TPC-DS: Total hot run time: 152323 ms |
ClickBench: Total hot run time: 24.13 s |
1f6e14e to
a75c214
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Static review complete for head a75c214be3134f02c7a59097612976c639cf3345 across all eight changed files. One new P2 diagnostic correctness issue is inline: an inner retry can retain actual row counts from a failed execution.
Critical checkpoints
- Goal and scope: Passing the current
Profileto manager removal addresses the leak when dispatch fails before a history entry exists. The added tests inspect missing and present history entries, retries, merged profiles, Flight paths, and mixed query/Broker Load pruning. The manager's ID rename follows the same profile ID source; no unrelated product behavior was found. - Concurrency and lifecycle: Query registration and asynchronous BE reports use the manager's execution index; removal now clears attached execution IDs. The manager's read/write lock still protects its indexes, and no new lock-order inversion was found. A live profile read can, however, populate derived row counts before an inner retry; the inline finding covers their missing reset. Inner MySQL retry, outer cloud replan, Flight request close/deferred execution, and Broker Load paths were traced separately.
- Conditions and parallel paths: The new retry flags preserve an open profile through a real retry and finalize terminal local attempts. The two previously reported P1 retry leaks (comments
4140101788and4143010094) are fixed on this head, so no existing blocking ID is carried. The existing P2 threads on outer statement duration (4174520825) and per-attempt phase times (4174520817) still apply and are not reposted. The outer plan-statistics, Flight abandoned-attempt, merged-profile, threshold, incomplete-profile, and Broker Load pruning threads are addressed by this diff. - Configuration, compatibility, and persistence: No new configuration, FE/BE variable, wire format, or persisted profile format was added. This change does not touch transaction or query-data writes. Profile file deletion/archiving now removes each attached execution by its own query ID, including decimal Broker Load profile IDs.
- Tests, observability, and performance: The new unit tests cover several cleanup branches, but they do not render the failed inner attempt before retry, which is needed to catch the inline issue. Profile text remains the relevant diagnostic surface. No separate performance regression or additional observability gap was substantiated. Review was static only; builds, tests, and CI results were not executed or independently validated here.
User focus: no additional focus was provided. The review converged after two rounds of normal and risk-focused inspection; all candidates were accepted, dismissed with code evidence, or covered by existing threads.
| if (willRetry) { | ||
| // Keep the statement open and its cumulative elapsed time, but discard the abandoned | ||
| // attempt so query retention and merged profiles only use the final execution profile. | ||
| profile.clearExecutionProfiles(); |
There was a problem hiding this comment.
[P2] Clear derived row counts when discarding an inner retry attempt. A live profile read after the first dispatch can populate rowsProducedMap and write actualRows into this same planner's physical plan. If a later getNext() RPC error triggers an inner retry, this call removes the failed execution but leaves those derived values. The next attempt reuses the plan, and merged rendering only putAlls new counters, so nodes without a new report still show the failed attempt's rows. Reset both the cached rows and the reused plan's actual-row statistics, and cover a retry after rendering the first attempt.
TPC-H: Total hot run time: 28009 ms |
TPC-DS: Total hot run time: 152209 ms |
ClickBench: Total hot run time: 23.92 s |
FE Regression Coverage ReportIncrement line coverage |
FE UT Coverage ReportIncrement line coverage |
a75c214 to
c28bccc
Compare
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68658 at c28bccc. The direct removeProfile(Profile) cleanup addresses the pre-registration ExecutionProfile leak, and retry cleanup now discards abandoned executions. Two distinct P2 diagnostic-profile defects remain and are reported inline.
Existing review context: r4174520825 (outer-retry statement duration) and r4174520817 (per-attempt phase times) still apply and are not reposted. The earlier P1 leak reports r4140101788 and r4143010094 are fixed at this head, so there are no still-applicable existing P0/P1 comments to carry. Other prior retry, Broker Load, merged-profile, Flight, and row-count threads were checked against this head and are either addressed or subsumed by the two inline findings.
Critical checkpoints:
- Goal, scope, and tests: The core leak path is addressed; the source change is focused on profile ownership, retry finalization, and manager bookkeeping. Added mock-driven tests cover missing history, multiple execution profiles, Broker Load pruning, inner and outer retries, merged rendering, Flight, and stored output. They do not exercise the two inline failure conditions. No builds or tests were run under this review contract.
- Concurrency and lifecycle: Query execution is serialized per connection, while backend reports and REST profile reads are asynchronous. Manager read/write locking fences live rendering against retry removal; late reports target the detached old execution object. Inner retry, outer replan, direct execute, local result, and Flight deferred finalization paths were traced. No separate lock-order or release defect was substantiated.
- Conditions, compatibility, and parallel paths: The
willRetryand outer handoff branches were checked against terminal and successful outcomes. No new configuration, FE/BE variable, wire or storage format, public REST behavior, or rolling-upgrade requirement was introduced. The REST authorization method rename preserves its lookup behavior. - Persistence, data writes, and errors: Profile spill and Broker Load ID pruning were reviewed; the attached execution IDs are removed by their own query IDs. There is no edit-log, transaction, committed-data, or FE/BE protocol change. Query failures still propagate; the inline findings concern incorrect retained diagnostics.
- Performance and observability: The new plan-row reset runs only on retries and no material new hot-path cost was found. The inline findings and the two existing timing threads affect profile accuracy. No additional logging or metric defect was substantiated.
User focus: no additional focus was provided. Review convergence completed in three rounds; the final changed-file sweep found no unresolved candidate beyond the two inline findings and the already reported P2 timing issues. This is a complete static review.
There was a problem hiding this comment.
Static review complete for PR #68658 at b730ef7. I found no new distinct inline issue. The stated FE leak path is addressed: removeProfile(Profile) removes attached execution IDs even when no history entry was published, and retry cleanup discards abandoned registrations before reusing the profile. The prior P1 inner retry and outer replan reports no longer apply to this head, so there are no existing blocking comment IDs to carry.
Two previously reported P2 issues still apply and are not reposted: an outer replan resets the profile's statement start, affecting total time and the retention threshold (duration thread); its shared SummaryProfile retains the first one-shot plan-finish timestamp across attempts, affecting schedule and fragment phase times (phase-time thread).
Review checkpoints:
- Goal, scope, and tests: the cleanup targets registration before history publication and remains focused on FE profile/retry lifecycle. Added unit tests cover absent/present history, multiple execution IDs, Broker Load pruning, query/INSERT/MERGE retry cases, rendering, and Flight. This was static inspection only; no tests or builds were run, and the tests are not claimed as executed validation.
- Concurrency and lifecycle: QeProcessor registration/reporting can overlap REST reads and background profile storage. Manager read/write locking fences manager-backed rendering against retry removal; Profile serializes its update/clear paths. The reviewed paths did not show a new lock-order cycle or blocking external I/O introduced under these locks. Inner and outer retries, terminal failures, empty commands, and Flight deferred close were traced; no new orphan was substantiated.
- Configuration, compatibility, and parallel paths: no new configuration item, FE/BE variable, wire or storage format, or rolling-upgrade protocol change. Query, command, Flight, Broker Load, REST lookup, and archive/delete paths were checked. The changed Java manager method references have no stale in-repository callers.
- Conditions, errors, and data safety: retry/finalization gates were traced through successful, cancelled, and planning-failure outcomes. This PR does not change transaction commit, data visibility, or EditLog behavior; profile storage pruning now removes each task's execution ID. No separate error-suppression or write-atomicity defect was substantiated.
- Test results, observability, and performance: no result files were changed. Profile output is the affected observability surface; the two known timing inaccuracies are linked above. Cleanup adds work only on retry and traverses the current plan to clear derived row counts; no material new hot-path cost was identified. No further distinct issue remained after the final changed-file sweep.
User focus: none was supplied beyond a complete PR review.
|
run buildall |
TPC-H: Total hot run time: 28972 ms |
TPC-DS: Total hot run time: 152108 ms |
ClickBench: Total hot run time: 24.52 s |
FE Regression Coverage ReportIncrement line coverage |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68658 at head 05808e3. All 12 changed files were covered, and the normal execution, normal profile, and separate risk-focused passes converged after one round with NO_NEW_VALUABLE_FINDINGS. No new inline findings remain.
Goal and tests: ProfileManager.removeProfile(Profile) now removes attached ExecutionProfiles even when no historical Profile entry was indexed, addressing the stated pre-dispatch leak. The added FE unit tests cover missing and existing summaries, retry cleanup, Broker Load pruning, phase resets, and rendered/stored profiles. This review inspected those tests but did not run builds or tests, as required by the review prompt.
Scope and correctness: The wider changes consistently clear abandoned retry execution/plan state and preserve the final attempt's summary. I traced direct queries, inner dispatch/fetch retries, outer cloud replans, INSERT/MERGE commands, Flight SQL, and Broker Load identity. The two existing P1 concerns (discussion IDs 4143010094 and 4140101788) are resolved by finalizing after retry decisions and removing attached execution IDs directly; neither remains a blocking comment on this head.
Concurrency and lifecycle: The executor, QeProcessor, profile manager GC/IO, and Flight request-close paths were checked. The examined manager-locked paths do not acquire the Profile monitor, and the Profile-to-manager cleanup has no inverse lock order in these paths. Flight terminal errors are finalized by FlightSqlConnectProcessor.close(); deferred Flight queries use their separate close path. No new registration, finalization, spill, or lock defect was substantiated.
Configuration, compatibility, and parallel paths: No new configuration or FE-BE wire variable was added. Retry fields are persisted with the existing Gson profile format, and old-profile field defaults remain available. Query, command, load, and REST lookup paths were checked; no transaction, EditLog, or user data-write behavior changed. The new conditions and retry-only resets have no substantiated performance regression.
Observability and remaining context: The added retry details and phase fields were checked against their producers and renderers. The QueryRetryTimes label counts failed attempts, including a first failure with no retry; the existing P2 thread at Profile.java:284 already asks for failed-attempt count to be distinguished from retry count, so this was not reposted. Other existing P2 concerns were checked against the current code and treated as duplicate context. There were no additional user focus points. This is a complete static review; executed validation is not claimed.
|
run buildall |
TPC-H: Total hot run time: 29271 ms |
TPC-DS: Total hot run time: 152104 ms |
ClickBench: Total hot run time: 24.74 s |
FE Regression Coverage ReportIncrement line coverage |
HappenLee
left a comment
HappenLee
left a comment
There was a problem hiding this comment.
LGTM. This correctly fixes the execution profile leak that occurs when dispatch fails before profile registration, preventing orphaned profile objects. The fix is clean and well-scoped.
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
LGTM at 05808e3. Rechecked both correctness and fix placement: ProfileManager removes attached execution IDs independently of history publication, while StmtExecutor distinguishes an abandoned attempt from statement completion. Keeping normal unregister separate preserves asynchronous BE profile reporting, and retry cleanup preserves Broker Load aggregation. No blocking correctness or architecture issue found. Validation: static review only; tests were inspected, not run locally.
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? Related PR: #68658 Problem Summary: CloudInsertProfileRetryTest uses the ProfileManager singleton, which starts a periodic storage thread. After an INSERT retry succeeds, this thread can persist the finished profile and clear its execution profiles before the test assertions run, causing intermittent failures. Use a dedicated ProfileManager without starting its periodic thread and redirect singleton access through a scoped static mock. Preserve the explicit background storage probe to verify that profiles remain ineligible for storage while the outer retry decision is pending.
…69894) ### What problem does this PR solve? Issue Number: N/A Problem Summary: Two FE unit tests depend on asynchronous work without owning its complete lifecycle: - `SplitProducerTest.readThatOutlivesItsStatementLoadsNothingIntoTheClosedScope` waits for the source-close signal and immediately checks the producer completion count. `SplitProducer.finish` invokes the completion callback after closing the source, so the assertion can run before that callback. Wait for the assignment termination future, which completes after both cleanup and the callback. - `StmtExecutorTest.testSuccessfulRetryRendersMergedProfileAndActualRows` registers its profile with the global background `ProfileManager` while also writing and reading the profile synchronously. Use a real manager whose daemon is not started, scoped to this test, so automatic spill, memory release and archival cannot interfere with the temporary storage lifecycle. Preserve the real persistence path and all merged-profile and actual-row assertions, and include the readback content in the failing assertion. The tests originated in #69718 and #68658 respectively. These test-only changes are independent of the Coordinator refactoring in #69830. The exact CI interleaving was not reproduced locally; both original tests also passed local repeated execution. No production behavior changes. ### Release note None ### Check List (For Author) - Test: On this independent master-based branch, `./run-fe-ut.sh --run 'SplitProducerTest,StmtExecutorTest'` passed all 81 cases (26 + 55). Before extraction, both target cases also passed 50 repetitions each (179 executions across both classes); FE Checkstyle passed with 0 violations. - Behavior changed: No - Does this need documentation: No
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
An ExecutionProfile is registered before query dispatch. If dispatch fails before the first
updateProfile(false)call, the corresponding Profile has not been added toprofileIdToProfileMap.When cleanup is invoked, the previous
removeProfileimplementation removed execution profiles only after finding a historical Profile entry. If no such entry existed, the ExecutionProfile remained inqueryIdToExecutionProfilesand continued to retain query state in FE memory. This was reproduced with a UDF JAR download failure: three failed queries left three ExecutionProfiles and no historical Profile entries.Pass the current Profile to
removeProfileand remove its ExecutionProfiles directly, regardless of whether a historical entry exists. Add tests covering a missing historical entry, an existing entry, and multiple ExecutionProfiles.Release note
Fix FE memory growth caused by execution profiles retained when query dispatch fails before profile registration.