[improvement](fe) Reuse boxed IDs in cloud tablet indexes - #66389
Merged
deardeng merged 2 commits intoAug 4, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
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.
The rebuild-side change is focused and preserves route equality, the public primitive method descriptor, and existing concurrency/lifecycle behavior. The boxed helper plus statRouteInfo callers do reuse identifier objects across the current/future global, table, and partition indexes, and the added test proves that helper behavior on empty maps.
One material performance gap remains: incremental direct moves, warmup, and rollback unbox the selected canonical tablet ID and re-box it inside updateBeToTablets; direct moves then retain distinct objects in the current and future families. Because the test calls the boxed helper directly, it does not cover this production path. I am requesting changes for the inline issue.
Critical checkpoints: the scope is otherwise small and clear; no new locking, static lifecycle, configuration, persistence, transaction, FE-BE, storage-format, or rolling-upgrade concern was found; all parallel rebuild and incremental paths were traced; no extra observability is needed for this allocation-only change. The review was static-only as required, so no builds or tests were run. There was no additional user-provided focus; the whole PR was reviewed.
Contributor
TPC-H: Total hot run time: 28451 ms |
Contributor
TPC-DS: Total hot run time: 169106 ms |
Contributor
ClickBench: Total hot run time: 23.89 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#61318 Problem Summary: Cloud tablet route rebuilding repeatedly boxes primitive backend, table, partition, index, and tablet IDs while inserting the same logical IDs into current and future global, table, and partition indexes. Hoist boxing to traversal callers and preserve selected boxed tablet IDs through direct transfers, warmup moves, and warmup rollbacks so all route index families reuse immutable Long references without changing routing or scheduling semantics. A single-threaded JDK 17 allocation model that keeps eager container candidates in both variants estimates that 4 million tablets across 4 clusters reduce cumulative allocation from 25.47 GiB to 19.76 GiB (22.42%) and approximate post-full-GC retained heap from 6.80 GiB to 4.94 GiB (27.37%). For IDs outside the Long cache, each direct incremental move additionally avoids two tablet wrapper allocations and one duplicate retained tablet wrapper; warmup and rollback each avoid one tablet wrapper allocation. These are path-level model and identity-test results, not production RSS measurements. ### Release note None ### Check List (For Author) - Test: Unit Test and Manual test\n - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest (13 tests passed)\n - mvn checkstyle:check -pl fe-core (0 violations)\n - Multi-scale JDK 17 eager-container allocation model at 0.2M, 0.4M, 0.8M, and 3.2M tablet-cluster pairs, three runs per scale\n- Behavior changed: No\n- Does this need documentation: No
Contributor
FE UT Coverage ReportIncrement line coverage |
deardeng
force-pushed
the
improve-cloud-rebalance-id-boxing-standalone
branch
from
August 3, 2026 14:56
96c3a48 to
a326d1c
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
gavinchou
previously approved these changes
Aug 3, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
approved
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
There was a problem hiding this comment.
Request changes: the boxed-ID reuse is correct within a single route-map generation, but one asynchronous warmup lifecycle still defeats the current/future sharing invariant.
Critical checkpoint conclusions:
- Goal and scope: The patch is focused and preserves equality-based routing behavior. Within one map generation, rebuild fills share one freshly boxed tablet across current/future indexes; selection, direct transfer, preheat, and same-generation rollback preserve the selected object, while completion carries the task object without reinserting route-map IDs. The inline issue identifies the remaining preheat -> later rebuild -> queued failure/rollback gap.
- Concurrency and lifecycle: The daemon rebuilds route maps before balancing, while warmup RPC failures are produced asynchronously and consumed later from
balanceImpl; that ordering is the source of the retained-identity split. The refactor introduces no additional lock-order, deadlock, or publication problem. - Compatibility and parallel paths: The public primitive
fillBeToTabletsdescriptor remains available, the boxed overload is package-private, and all repository callers resolve unambiguously. Full rebuild, direct transfer, async/sync warmup, completion, failure rollback, and smooth-upgrade-triggered rebuild paths were reviewed. No additional parallel-path issue was found. - Correctness, error handling, persistence, configuration, and observability: Numeric routing semantics, metadata/edit-log behavior, and existing failure reporting are unchanged. No configuration, FE/BE protocol, storage format, or observability change is introduced, and no separate issue was found in these areas.
- Performance: Sharing across the global/table/partition and current/future indexes works at the intended tablet-cluster insertion granularity except for the accepted cross-generation rollback issue.
- Tests and results: The above-cache unit tests cover the helper, direct transfer, preheat, and immediate rollback, but not preheat -> full rebuild -> asynchronous failure/revert. The PR body's reported 10-test class run predates the three newly added lifecycle tests at the authoritative head, so this static review does not treat that result as current-head execution evidence. At payload freeze, live CheckStyle is successful, FE UT is pending, and
check_coverage_feis failing. Per the review constraints, no builds or tests were run here. - User focus: No additional review focus was provided; the complete PR was reviewed.
- Review completion: Complete after three bounded normal/risk-focused review rounds against the frozen head, diff, existing-thread set, ledger, summary, and final inline wording. One distinct issue is submitted; the earlier same-generation thread was deduplicated.
Contributor
TPC-H: Total hot run time: 28413 ms |
Contributor
TPC-DS: Total hot run time: 168974 ms |
Contributor
ClickBench: Total hot run time: 23.8 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#66389 Problem Summary: When an asynchronous warmup task survives into a later rebalance round, statRouteInfo rebuilds current and future route maps with a newly boxed tablet ID. If the queued warmup failure is then rolled back, future maps reinsert the task's older boxed ID while current maps retain the rebuilt object. Numeric routing stays correct, but current and future indexes retain duplicate Long wrappers until the next rebuild, defeating boxed-ID sharing. Reuse the inflight task's canonical tablet ID for both map families during rebuild, avoiding one duplicate retained wrapper per affected failed inflight tablet without changing route or scheduling semantics. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest (14 tests passed) - cd fe && mvn checkstyle:check -pl fe-core (0 violations) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
github-actions
Bot
removed
the
approved
Contributor
There was a problem hiding this comment.
Review opinion: no blocking issue remains at head dd6909afab54253408756d1408964855e8612b76; this is a comment-only review with zero new inline comments.
Critical checkpoint conclusions:
- Goal and proof: The patch hoists ID boxing to traversal/update callers and preserves the selected
Longthrough full route rebuilds, direct transfer, preheat, rollback, and the task-survives-rebuild failure path. The above-cache unit cases cover helper insertion, direct and warmup moves, same-generation rollback, and preheat -> rebuild -> queued failure -> rollback. - Scope and compatibility: The production change is focused on boxed-ID plumbing. The public primitive
fillBeToTablets(long, ...)descriptor remains intact; other changed descriptors are non-public and have no outside repository callers. No configuration, FE-BE protocol, storage format, persistence schema, rolling-upgrade, or documentation change is introduced. - Concurrency and lifecycle: The main daemon, warmup batch/RPC workers, and checker paths were traced end to end. Task fields are assigned before concurrent-map publication, warmup-task fields are final, and
Longis immutable. The diff adds no lock, lock-order, deadlock, static-initialization, or ownership-cycle risk. The current head addresses both existing tablet-identity threads, including the later-generation rollback ordering. - Correctness and parallel paths: Routing remains equality-based; no reference comparison affects scheduling or metadata. Global/table/partition rebuilds, direct transfer, async/sync warmup, completion/timeout, failure rollback, balance-type/backend-loss removal, colocate/secondary statistics, and smooth-upgrade rebuild were reviewed. Existing error, EditLog, and observability behavior is unchanged and sufficient for this allocation-only change.
- Performance: Each logical fill now reuses one boxed object across global, table, and partition operations, and incremental paths keep the selected tablet object across current/future families. No material regression or unsupported performance claim was substantiated.
- Tests and results: The identity assertions use IDs outside the mandated
Longcache and are meaningful against the primitive base paths. This review was static-only as required, so no build or test was run here; live CheckStyle was passing at payload freeze. - User focus: No additional user-provided focus was present, so the complete PR was reviewed.
- Review completion: Three bounded normal and risk-focused convergence rounds were completed against the same head, existing-thread set, adjudicated ledger, and frozen zero-inline payload; every final-round reviewer returned
NO_NEW_VALUABLE_FINDINGS.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28666 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 166911 ms |
Contributor
ClickBench: Total hot run time: 23.89 s |
gavinchou
approved these changes
Aug 4, 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
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 rebuilding repeatedly boxes primitive backend, table, partition, index, and tablet IDs while inserting the same logical IDs into current and future global, table, and partition indexes.
This PR contains two related improvements:
Reuse boxed IDs across route indexes
Hoist boxing to the traversal and incremental update callers, then pass the same immutable
Longreferences through a boxed helper overload. Preserve the public primitive method descriptor and the existingputIfAbsentcontainer implementation so the optimizationapplies directly to master without depending on lazy container creation.
Preserve in-flight tablet IDs across route rebuilds
An asynchronous warmup task may remain in flight when the next rebalance round rebuilds route information. Previously, the rebuild stored a newly boxed tablet ID in the current and future indexes. If the warmup subsequently failed, rollback restored the original boxed
ID held by the task only to the future indexes, while the current indexes retained the newly boxed instance.
The fix reuses
InfightTask.pickedTabletIdwhen rebuilding both current and future route indexes. This preserves boxed-ID identity across the global, table-level, and partition-level indexes without changing routing decisions, destination backends, or schedulingsemantics. It also prevents one additional
Longwrapper from being retained for each affected failed in-flight tablet until the next route rebuild.A single-threaded JDK 17 allocation model that keeps eager container candidates in both variants estimates that 4 million tablets across 4 clusters reduce cumulative allocation from 25.47 GiB to 19.76 GiB (22.42%) and approximate post-full-GC retained heap from 6.80 GiB
to 4.94 GiB (27.37%). These are path-level model estimates rather than production RSS measurements.
The allocation estimates cover the boxed-ID reuse optimization. The cross-generation warmup rollback fix provides an additional lifecycle correctness guarantee and is not included in those estimates.
related PR #66378
Release note
None
Check List (For Author)
Test: Unit Test and Manual test
./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest (10 tests passed)
./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest#testFillBeToTabletsReusesBoxedIdsAcrossIndexes (RED before fix, passed after fix)
mvn checkstyle:check -pl fe-core (0 violations)
Multi-scale JDK 17 eager-container allocation model at 0.2M, 0.4M, 0.8M, and 3.2M tablet-cluster pairs, three runs per scale
Behavior changed: No
Does this need documentation: No
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)