[improvement](fe) Release cloud tablet scheduling indexes after balance - #66451
Open
deardeng wants to merge 2 commits into
Open
[improvement](fe) Release cloud tablet scheduling indexes after balance#66451deardeng wants to merge 2 commits into
deardeng wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#66378, apache#66389 Problem Summary: Cloud tablet route rebuilding retains the current and future table-level and partition-level scheduling indexes for the entire sleep interval after each balancing round, although later status checks and external readers only need the global indexes. At 4 million tablets across 4 clusters, a single-threaded JDK 17 path-level model estimates that releasing these four nested graphs reduces approximate post-full-GC retained route-index heap from 4.66 GiB to 1.35 GiB, a 3.31 GiB or 70.96% reduction. Cumulative route-index construction allocation remains 7.36 GiB, so this change reduces between-round retention rather than allocation volume or in-round peak memory. Replace the four top-level maps in a finally block after balancing so all exit paths release the old graphs without an O(N) clear traversal, while preserving current and future global routes. The next route-statistics pass rebuilds the scheduling indexes before they are used again. These numbers are model estimates, not production RSS measurements. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest (16 tests passed) - mvn checkstyle:check -pl fe-core (0 violations) - 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.
Review opinion: Request changes.
One material lifecycle gap remains: the new cleanup fence covers every exit from performBalancing(), but not smooth-upgrade migration failures after route indexes have already been rebuilt. The inline comment gives the concrete failure path and the recommended wider fence.
Critical checkpoint conclusions:
- Task goal and focus: replacing the four detailed current/future table/partition maps is an O(1) way to reduce between-round retained heap, and the normal balancing path accomplishes that goal. The accepted exception path makes the stated all-exit-path result incomplete. There was no additional user-provided focus.
- Scope and parallel paths: the production change is small and focused; all four detailed current/future maps are released together, while current/future global, secondary, and colocate maps needed by later readers remain intact.
- Concurrency and lifecycle: the daemon thread owns detailed-map construction, balancing, rollback application, and release. Warmup executor/checker threads keep ID-only task state and do not retain or read the released graphs. Normal returns, active-phase early returns, and exceptions from
performBalancing()are safe; the pre-trymigration exception is not. - Configuration, compatibility, and FE/BE contracts: no configuration, persisted format, function symbol, RPC/thrift contract, or rolling-upgrade compatibility surface changes.
- Conditions, transactions, and data writes: routing/edit-log behavior is otherwise unchanged; post-balance decommission checks use the preserved current global map. No new transaction or persistence operation is introduced.
- Tests and results: the helper-level unit test proves map replacement, global-map identity, and direct next-pass reconstruction, and adjacent coverage exercises warmup rollback after rebuild. It does not execute the production
try/finallyor its migration exception path. This review was static-only and ran no tests; the author reports 16 tests passed, matching the 16@Testannotations currently in the class, but that result was not independently rerun here. - Observability and performance: no additional metrics/logging are needed for this lifetime-only change. Top-level replacement avoids an O(N) clear and the numerical heap claim is explicitly identified as a model estimate. No other correctness or performance issue was substantiated.
Review convergence is complete for the frozen diff and comment set: the final normal and risk-focused reviewers found no additional valuable findings, every candidate was independently adjudicated, and existing-thread duplicate suppression found no prior inline discussion.
Contributor
TPC-H: Total hot run time: 28961 ms |
Contributor
TPC-DS: Total hot run time: 166387 ms |
Contributor
ClickBench: Total hot run time: 24.01 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#66451, apache#66378, apache#66389 Problem Summary: Cloud tablet route indexes are rebuilt before smooth-upgrade migration, but the scheduling-index cleanup fence previously covered only performBalancing(). If migration or the optional second route rebuild threw after the first rebuild, the daemon propagated the failure without replacing the four current/future table-level and partition-level indexes, retaining their tablet membership graphs until a later cycle. Move the existing try/finally boundary before the first statRouteInfo() call so route rebuild, smooth-upgrade migration, the optional second rebuild, and balancing all release the scheduling-only indexes on every exit path. Global route indexes and exception propagation remain unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest#testRunAfterCatalogReadyReleasesSchedulingIndexesWhenMigrationFails - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest (17 tests passed) - mvn checkstyle:check -pl fe-core (0 violations) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Contributor
FE UT Coverage ReportIncrement line coverage |
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
2 participants
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.
Related PR: #66378, #66389
Problem Summary: Cloud tablet route rebuilding retains the current and future table-level and partition-level scheduling indexes for the entire sleep interval after each balancing round, although later status checks and external readers only need the global indexes. At 4 million tablets across 4 clusters, a single-threaded JDK 17 path-level model estimates that releasing these four nested graphs reduces approximate post-full-GC retained route-index heap from 4.66 GiB to 1.35 GiB, a 3.31 GiB or 70.96% reduction. Cumulative route-index construction allocation remains 7.36 GiB, so this change reduces between-round retention rather than allocation volume or in-round peak memory. Replace the four top-level maps in a finally block after balancing so all exit paths release the old graphs without an O(N) clear traversal, while preserving current and future global routes. The next route-statistics pass rebuilds the scheduling indexes before they are used again. These numbers are model estimates, not production RSS measurements.
Release note
None
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)