[fix](fe) Avoid eager allocations in cloud tablet indexes - #66378
Open
deardeng wants to merge 1 commit into
Open
[fix](fe) Avoid eager allocations in cloud tablet indexes#66378deardeng wants to merge 1 commit into
deardeng wants to merge 1 commit into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#61318 Problem Summary: Cloud tablet route-cache rebuilding calls fillBeToTablets for current and future placements. The previous putIfAbsent calls eagerly constructed up to six candidate maps and sets for every placement even when the keys already existed, and then performed separate lookups. Use the containers returned by computeIfAbsent at every level and add a focused unit test that verifies all six container paths while preserving the global, table, and partition index contents. An isomorphic multi-scale JVM allocation model with 2 million entities, four-route fan-out, current/future passes, and three index scopes estimated cumulative allocation per modeled rebuild at 12.52 GiB before and 8.59 GiB after the change, saving 3.93 GiB (31.4%). For 4 million tablets, the fill-path cumulative-allocation reduction is expected to remain about 31% under comparable topology, while the absolute GiB saving depends on cluster and replica fan-out. The retained graph remained about 3.76 GiB and the peak proxy about 966 MiB, so this change does not claim a comparable steady-state heap or process-wide reduction. Timing samples were noisy, so no throughput improvement is claimed. ### Release note None ### Check List (For Author) - Test: Unit Test / Manual benchmark - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest#testFillBeToTabletsUsesComputedContainers - cd fe && mvn checkstyle:check -pl fe-core - Multi-scale isomorphic JVM allocation model: 12.52 GiB to 8.59 GiB cumulative allocation per modeled rebuild - 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 |
1 similar comment
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: looks good. I found no blocking issue or substantiated inline finding.
Critical checkpoint conclusions:
- Goal and proof: the change replaces all six eager candidate-container allocations and follow-up lookups in
fillBeToTabletswith the installed values returned bycomputeIfAbsent. The focused unit test distinguishes the old implementation at all six levels and verifies the global, table, and partition/index tablet contents. - Scope and clarity: the production change is small, focused, and reuses one existing helper for current and future route-cache rebuilds, direct balance moves, and warmup rollback. No parallel fill path was missed.
- Concurrency: route-map writes occur in the single
CloudTabletRebalancermaster-daemon flow; warmup workers enqueue failures for that daemon rather than mutating these maps directly. The mapping functions are short, non-null, and non-reentrant, andcomputeIfAbsentreturns the installed container identity before the nested mutation. No outer-entry removal or replacement can detach that identity. - Lifecycle: per-cycle maps are built and then published as before; later move/rollback operations mutate the same installed concurrent sets. No initialization, ownership, cleanup, or release behavior changes.
- Conditions, configuration, compatibility, and persistence: no new condition, configuration, protocol, symbol, storage format, FE/BE variable, EditLog, transaction, or data-write behavior is introduced. Error handling and rolling-upgrade behavior are unchanged.
- Performance: present keys no longer allocate discarded maps/sets or require a separate explicit lookup. The change adds no scan, retained state, or heavy work under a lock, and it appropriately makes no throughput claim.
- Tests and result review: the new single-threaded counter test is deterministic, fails the prior
putIfAbsent/getimplementation, exercises absent and present backend-set keys plus present nested containers, and checks all three result scopes. The live CheckStyle check is green. The FE unit-test job is still pending; no local build or test was run because this review runner explicitly prohibits builds. - Observability: this is a behavior-preserving local allocation refactor with existing operational paths unchanged, so no new log or metric is warranted.
- User focus: no additional focus was provided; the complete PR was reviewed.
Review completion: Round 1 included a complete-review subagent and a separate risk-focused concurrency/test-oracle subagent. Both returned NO_NEW_VALUABLE_FINDINGS; the final changed-file, candidate, live-thread, and head/base sweeps found no unresolved point. Inline comments: zero.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28710 ms |
Contributor
TPC-DS: Total hot run time: 170674 ms |
Contributor
ClickBench: Total hot run time: 24 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
gavinchou
approved these changes
Aug 3, 2026
16 tasks
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone 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
3 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.
Problem Summary: Cloud tablet route-cache rebuilding calls fillBeToTablets for current and future placements. The previous putIfAbsent calls eagerly constructed up to six candidate maps and sets for every placement even when the keys already existed, and then performed separate lookups. Use the containers returned by computeIfAbsent at every level and add a focused unit test that verifies all six container paths while preserving the global, table, and partition index contents.
An isomorphic multi-scale JVM allocation model with 2 million entities, four-route fan-out, current/future passes, and three index scopes estimated cumulative allocation per modeled rebuild at 12.52 GiB before and 8.59 GiB after the change, saving 3.93 GiB (31.4%). For 4 million tablets, the fill-path cumulative-allocation reduction is expected to remain about 31% under comparable topology, while the absolute GiB saving depends on cluster and replica fan-out. The retained graph remained about 3.76 GiB and the peak proxy about 966 MiB, so this change does not claim a comparable steady-state heap or process-wide reduction. Timing samples were noisy, so no throughput improvement is claimed.
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)