[improvement](fe) Reuse boxed IDs in cloud tablet indexes - #66389
Open
deardeng wants to merge 1 commit into
Open
[improvement](fe) Reuse boxed IDs in cloud tablet indexes#66389deardeng wants to merge 1 commit into
deardeng wants to merge 1 commit into
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
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.
| InfightTask task = tabletToInfightTask.get(taskKey); | ||
| long futureBeId = task == null ? beId : task.destBe; | ||
| fillBeToTablets(beId, table.getId(), partition.getId(), index.getId(), tabletId, | ||
| Long futureBeId = task == null ? beId : Long.valueOf(task.destBe); |
Contributor
There was a problem hiding this comment.
[P2] Preserve inflight identity across route rebuilds
This is distinct from the existing thread's immediate unbox/rebox path: it requires the asynchronous task to survive into a later daemon run. statRouteInfo() creates a fresh above-cache Long and installs it in both rebuilt map families, while WarmupTabletTask still holds the previous selected object. If the queued RPC failure is then consumed by balanceImpl, revertWarmupState() removes the rebuilt object only from the future sets and reinserts the stale task object; the current sets keep the fresh reference. The new rollback test reverts within one map generation, so it misses this ordering. Please reuse the inflight task's canonical pickedTabletId for both fills during rebuild (or recover the canonical current-map object during rollback), and cover preheat -> rebuild -> failure/revert.
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 |
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. Hoist boxing to the traversal and incremental update callers and pass the same immutable Long references through a boxed helper overload. Preserve the public primitive method descriptor and the existing putIfAbsent container implementation so this change applies directly to master without depending on lazy container creation. 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, not production RSS measurements.
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)