Repository navigation
[Fix](test) Isolate profile manager in cloud insert retry tests - #69015
Merged
Merged
Conversation
linrrzqqq
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
October 13, 2026 09:10
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
/review |
Collaborator
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed PR #69015 at f5983f2 using the supplied authoritative diff. No new actionable findings were identified across the single changed file, all three hunks, and all 14 INSERT/MERGE cases. Both independent reviewers returned NO_NEW_VALUABLE_FINDINGS in round 1; the main review verified their evidence and completed the final sweep.
The isolation matches the reported failure mechanism: constructing TestProfileManager does not start its periodic storage daemon, and the scoped getter mock routes registration, retry reset, finalization and cleanup to that instance. The explicit spill worker accesses the Profile directly and finishes before retry continues, so it still checks storage eligibility while the outer decision is pending. Internal manager executors receive no tasks on these paths; repeated constructor registrations replace the same two pool keys and do not retain all case managers.
Critical checkpoint conclusions:
- Goal and proof: the source removes the uncontrolled periodic spill/release race while retaining the retry-window probe and final persistence assertions. Runtime success is unverified.
- Scope and clarity: the change is confined to one test; helper extraction preserves case selection, execution and assertions, with a minimal subclass for the protected constructor.
- Concurrency and locking: registration, finalization and callbacks are synchronous on the test thread; the worker probe is ordered by Future.get. Existing manager locks remain in place, with no new nested lock, lock-order change, or IO under a new lock.
- Lifecycle and initialization: try-with-resources closes both static mocks; existing cleanup runs while the manager mock remains active and restores configuration/session/thread-local state. The test manager and its internal pools start no threads on this path; no new ownership cycle is introduced.
- Configuration: no item is added; temporary cloud/retry settings retain their existing restoration.
- Compatibility: no production API, symbol, storage format or rolling-upgrade behavior changes.
- Thrift/protobuf: no definition, field ID or enum value changes.
- Parallel paths: the shared wrapper covers all ten INSERT and four MERGE variants, including the real outer execution and empty-return MERGE case.
- Conditional checks: no new condition is introduced; existing outcome and retry branches are preserved.
- Test coverage: the JUnit 5 cases retain success, session restoration, empty input, planning failure, exhaustion, cancellation, direct execution, RPC and non-replan failure checks. Required Mockito core/inline dependencies already exist; no added style issue was identified by inspection.
- Test results: no golden/output file changes. ZIP readback assertions still compare retry details, effective session settings and exclusion of abandoned execution state; none was executed in this review.
- Observability: existing assertions and profile state/retry details cover the test behavior; no additional production logging or metric is needed.
- Transactions and persistence: EditLog, transaction ownership and failover behavior are unchanged. Real profile write/read assertions remain within the isolated test scope.
- Data writes and atomicity: production data writes and crash semantics are unchanged; test writes remain limited to temporary profile ZIP files.
- FE-BE variables: no transmitted variable or send path changes; mock backend reports and query options are preserved.
- Performance: no production hot-path work is added; manager pools are lazy and unused by these cases, with bounded registry retention.
- Other issues and core invariants: the final recheck found no introduced setup/cleanup, static-mock nesting, accessibility or callback defect. Production data visibility, visible versions, delete bitmaps and BE memory accounting are unaffected; failure assertions remain intact.
No existing inline comments or additional user focus were present. Both initial risk concerns were dismissed with concrete call-path and resource-lifecycle evidence; no unresolved source-review candidate or coverage gap remains. This was a static review only: no builds, tests or checkstyle checks ran, and CI results and empirical flake elimination were not verified.
Review converged after 1 round(s).
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28606 ms |
Contributor
TPC-DS: Total hot run time: 152527 ms |
Contributor
ClickBench: Total hot run time: 24.52 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28696 ms |
Contributor
TPC-DS: Total hot run time: 151634 ms |
Contributor
ClickBench: Total hot run time: 24.38 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
3 similar comments
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
924060929
approved these changes
Oct 14, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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: close #xxx
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.
Release note
None
Check List (For Author)