Repository navigation
[test](fe) Stabilize scan termination and retry profile assertions - #69894
Merged
CalvinKirs merged 1 commit intoNov 2, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: N/A Problem Summary: FE UT reported a split producer completion assertion before its completion callback finished and a retry profile assertion after persistence. Wait for the scan assignment termination future before checking completion. Isolate the retry profile test with a real ProfileManager whose daemon is not started, so background spill and memory release cannot race the test's synchronous persistence and readback. Keep all merged-profile and actual-row checks. ### Release note None ### Check List (For Author) - Test: Both affected classes passed with each target test repeated 50 times, 179 executions total; FE Checkstyle passed. The original CI interleaving did not reproduce in the local baseline stress run. No production code changed. - Behavior changed: No - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR 69894 at head 384e24a found no new issues. The change is limited to two FE tests. In SplitProducerTest, the release latch precedes a bounded termination-future wait, which follows the producer's source-close and completion path and removes an assertion race around late connector reads. In StmtExecutorTest, the fresh unstarted ProfileManager is used inside a scoped static mock; all exercised profile and query-manager calls are synchronous on the test thread, while the nested finally unregisters both attempts and removes the profile before the mock closes. The storage checks remain synchronous and validate both merged in-memory and serialized output.
Critical checkpoints: the task goal is achieved by making cleanup and profile-storage assertions deterministic; the test edits are focused; the split cancellation path and profile retry path were traced through their upstream/downstream lifecycle and locking boundaries; no new configuration, protocol, persistence, or production concurrency path is introduced; no parallel path or error-handling defect was found; and the existing global/resource cleanup is preserved. The user focus file specified no additional focus.
Validation was static only as required by the review contract; no builds or tests were executed. Existing inline review threads and blocking IDs are empty. Three reviewers completed the required round-1 coverage and returned NO_NEW_VALUABLE_FINDINGS, with all initial risk items dismissed using concrete control-flow evidence. Review converged in one round.
Review converged after 1 round(s).
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 28832 ms |
Contributor
TPC-DS: Total hot run time: 151643 ms |
Contributor
ClickBench: Total hot run time: 24.35 s |
Contributor
Author
|
run cloud_p0 |
Contributor
Author
|
run check_coverage |
CalvinKirs
approved these changes
Nov 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.readThatOutlivesItsStatementLoadsNothingIntoTheClosedScopewaits for the source-close signal and immediately checks the producer completion count.SplitProducer.finishinvokes 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.testSuccessfulRetryRendersMergedProfileAndActualRowsregisters its profile with the global backgroundProfileManagerwhile 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)
./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.